Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Oct 2022 18:55:23 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Ian Kent <raven@themaw.net>
Cc:     Al Viro <viro@ZenIV.linux.org.uk>,
        Siddhesh Poyarekar <siddhesh@gotplt.org>,
        David Howells <dhowells@redhat.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Carlos Maiolino <cmaiolino@redhat.com>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [REPOST PATCH v3 2/2] vfs: parse: deal with zero length string
 value
Message-Id: <20221017185523.22f43b5d7f9fee1e1e3d872f@linux-foundation.org>
In-Reply-To: <166365878918.39016.12757946948158123324.stgit@donald.themaw.net>
References: <166365872189.39016.10771273319597352356.stgit@donald.themaw.net>
        <166365878918.39016.12757946948158123324.stgit@donald.themaw.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 20 Sep 2022 15:26:29 +0800 Ian Kent <raven@themaw.net> wrote:

> Parsing an fs string that has zero length should result in the parameter
> being set to NULL so that downstream processing handles it correctly.
> For example, the proc mount table processing should print "(none)" in
> this case to preserve mount record field count, but if the value points
> to the NULL string this doesn't happen.
> 
> ...
>
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -197,6 +197,8 @@ int fs_param_is_bool(struct p_log *log, const struct fs_parameter_spec *p,
>  		     struct fs_parameter *param, struct fs_parse_result *result)
>  {
>  	int b;
> +	if (param->type == fs_value_is_empty)
> +		return 0;
>  	if (param->type != fs_value_is_string)
>  		return fs_param_bad_value(log, param);
>  	if (!*param->string && (p->flags & fs_param_can_be_empty))
> @@ -213,6 +215,8 @@ int fs_param_is_u32(struct p_log *log, const struct fs_parameter_spec *p,
>  		    struct fs_parameter *param, struct fs_parse_result *result)
>  {
>  	int base = (unsigned long)p->data;
> +	if (param->type == fs_value_is_empty)
> +		return 0;
>  	if (param->type != fs_value_is_string)
>  		return fs_param_bad_value(log, param);
>  	if (!*param->string && (p->flags & fs_param_can_be_empty))
>
> [etcetera]

This feels wrong.  Having to check for fs_value_is_empty in so many
places makes me think "we just shouldn't have got this far".  Am I
right for once?