Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 25 Oct 2021 13:12:29 +0200
From:   Carlos Maiolino <cmaiolino@redhat.com>
To:     Lukas Czerner <lczerner@redhat.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        linux-fsdevel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v3 01/13] fs_parse: allow parameter value to be empty
Message-ID: <20211025111229.zvk6np675v5fip2j@andromeda.lan>
Mail-Followup-To: Lukas Czerner <lczerner@redhat.com>,
        linux-ext4@vger.kernel.org, tytso@mit.edu,
        linux-fsdevel@vger.kernel.org, Al Viro <viro@zeniv.linux.org.uk>
References: <20211021114508.21407-1-lczerner@redhat.com>
 <20211021114508.21407-2-lczerner@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211021114508.21407-2-lczerner@redhat.com>
Precedence: bulk

On Thu, Oct 21, 2021 at 01:44:56PM +0200, Lukas Czerner wrote:
> Allow parameter value to be empty by spcifying fs_param_can_be_empty
					^ specifying


Other than that, the patch looks fine:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> +	if (!*param->string && (p->flags & fs_param_can_be_empty))

> +		return 0;
>  	b = lookup_constant(bool_names, param->string, -1);
>  	if (b == -1)
>  		return fs_param_bad_value(log, param);
> @@ -211,8 +213,11 @@ 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_string ||
> -	    kstrtouint(param->string, base, &result->uint_32) < 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))
> +		return 0;
> +	if (kstrtouint(param->string, base, &result->uint_32) < 0)
>  		return fs_param_bad_value(log, param);
>  	return 0;
>  }
> @@ -221,8 +226,11 @@ EXPORT_SYMBOL(fs_param_is_u32);
>  int fs_param_is_s32(struct p_log *log, const struct fs_parameter_spec *p,
>  		    struct fs_parameter *param, struct fs_parse_result *result)
>  {
> -	if (param->type != fs_value_is_string ||
> -	    kstrtoint(param->string, 0, &result->int_32) < 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))
> +		return 0;
> +	if (kstrtoint(param->string, 0, &result->int_32) < 0)
>  		return fs_param_bad_value(log, param);
>  	return 0;
>  }
> @@ -231,8 +239,11 @@ EXPORT_SYMBOL(fs_param_is_s32);
>  int fs_param_is_u64(struct p_log *log, const struct fs_parameter_spec *p,
>  		    struct fs_parameter *param, struct fs_parse_result *result)
>  {
> -	if (param->type != fs_value_is_string ||
> -	    kstrtoull(param->string, 0, &result->uint_64) < 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))
> +		return 0;
> +	if (kstrtoull(param->string, 0, &result->uint_64) < 0)
>  		return fs_param_bad_value(log, param);
>  	return 0;
>  }
> @@ -244,6 +255,8 @@ int fs_param_is_enum(struct p_log *log, const struct fs_parameter_spec *p,
>  	const struct constant_table *c;
>  	if (param->type != fs_value_is_string)
>  		return fs_param_bad_value(log, param);
> +	if (!*param->string && (p->flags & fs_param_can_be_empty))
> +		return 0;
>  	c = __lookup_constant(p->data, param->string);
>  	if (!c)
>  		return fs_param_bad_value(log, param);
> @@ -255,7 +268,8 @@ EXPORT_SYMBOL(fs_param_is_enum);
>  int fs_param_is_string(struct p_log *log, const struct fs_parameter_spec *p,
>  		       struct fs_parameter *param, struct fs_parse_result *result)
>  {
> -	if (param->type != fs_value_is_string || !*param->string)
> +	if (param->type != fs_value_is_string ||
> +	    (!*param->string && !(p->flags & fs_param_can_be_empty)))
>  		return fs_param_bad_value(log, param);
>  	return 0;
>  }
> @@ -275,7 +289,8 @@ int fs_param_is_fd(struct p_log *log, const struct fs_parameter_spec *p,
>  {
>  	switch (param->type) {
>  	case fs_value_is_string:
> -		if (kstrtouint(param->string, 0, &result->uint_32) < 0)
> +		if ((!*param->string && !(p->flags & fs_param_can_be_empty)) ||
> +		    kstrtouint(param->string, 0, &result->uint_32) < 0)
>  			break;
>  		if (result->uint_32 <= INT_MAX)
>  			return 0;
> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
> index aab0ffc6bac6..f103c91139d4 100644
> --- a/include/linux/fs_parser.h
> +++ b/include/linux/fs_parser.h
> @@ -42,7 +42,7 @@ struct fs_parameter_spec {
>  	u8			opt;	/* Option number (returned by fs_parse()) */
>  	unsigned short		flags;
>  #define fs_param_neg_with_no	0x0002	/* "noxxx" is negative param */
> -#define fs_param_neg_with_empty	0x0004	/* "xxx=" is negative param */
> +#define fs_param_can_be_empty	0x0004	/* "xxx=" is allowed */
>  #define fs_param_deprecated	0x0008	/* The param is deprecated */
>  	const void		*data;
>  };
> -- 
> 2.31.1
> 

-- 
Carlos