Re: [PATCH v8 1/3] fs_parser: add fsparam_u16 helper

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 10/11/24 21:13, Joanne Koong wrote:
> Add a fsparam helper for unsigned 16 bit values.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fs_parser.c            | 14 ++++++++++++++
>  include/linux/fs_parser.h |  9 ++++++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fs_parser.c b/fs/fs_parser.c
> index 24727ec34e5a..0e06f9618c89 100644
> --- a/fs/fs_parser.c
> +++ b/fs/fs_parser.c
> @@ -210,6 +210,20 @@ int fs_param_is_bool(struct p_log *log, const struct fs_parameter_spec *p,
>  }
>  EXPORT_SYMBOL(fs_param_is_bool);
>  
> +int fs_param_is_u16(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)
> +		return fs_param_bad_value(log, param);
> +	if (!*param->string && (p->flags & fs_param_can_be_empty))
> +		return 0;
> +	if (kstrtou16(param->string, base, &result->uint_16) < 0)
> +		return fs_param_bad_value(log, param);
> +	return 0;
> +}
> +EXPORT_SYMBOL(fs_param_is_u16);
> +
>  int fs_param_is_u32(struct p_log *log, const struct fs_parameter_spec *p,
>  		    struct fs_parameter *param, struct fs_parse_result *result)
>  {
> diff --git a/include/linux/fs_parser.h b/include/linux/fs_parser.h
> index 6cf713a7e6c6..1c940756300c 100644
> --- a/include/linux/fs_parser.h
> +++ b/include/linux/fs_parser.h
> @@ -26,9 +26,10 @@ typedef int fs_param_type(struct p_log *,
>  /*
>   * The type of parameter expected.
>   */
> -fs_param_type fs_param_is_bool, fs_param_is_u32, fs_param_is_s32, fs_param_is_u64,
> -	fs_param_is_enum, fs_param_is_string, fs_param_is_blob, fs_param_is_blockdev,
> -	fs_param_is_path, fs_param_is_fd, fs_param_is_uid, fs_param_is_gid;
> +fs_param_type fs_param_is_bool, fs_param_is_u16, fs_param_is_u32, fs_param_is_s32,
> +	fs_param_is_u64, fs_param_is_enum, fs_param_is_string, fs_param_is_blob,
> +	fs_param_is_blockdev, fs_param_is_path, fs_param_is_fd, fs_param_is_uid,
> +	fs_param_is_gid;
>  
>  /*
>   * Specification of the type of value a parameter wants.
> @@ -55,6 +56,7 @@ struct fs_parse_result {
>  	union {
>  		bool		boolean;	/* For spec_bool */
>  		int		int_32;		/* For spec_s32/spec_enum */
> +		u16             uint_16;	/* For spec_u16 *
>  		unsigned int	uint_32;	/* For spec_u32{,_octal,_hex}/spec_enum */

Given fs_param_is_u16() has "int base = (unsigned long)p->data;", 
shouldn't the comment also mention ",_octal,_hex}/spec_enum"?
Or should the function get slightly modified to always use a base of 10?
Or 0 with auto detection as for fs_param_is_u64()?

>  		u64		uint_64;	/* For spec_u64 */
>  		kuid_t		uid;
> @@ -119,6 +121,7 @@ static inline bool fs_validate_description(const char *name,
>  #define fsparam_flag_no(NAME, OPT) \
>  			__fsparam(NULL, NAME, OPT, fs_param_neg_with_no, NULL)
>  #define fsparam_bool(NAME, OPT)	__fsparam(fs_param_is_bool, NAME, OPT, 0, NULL)
> +#define fsparam_u16(NAME, OPT)	__fsparam(fs_param_is_u16, NAME, OPT, 0, NULL)
>  #define fsparam_u32(NAME, OPT)	__fsparam(fs_param_is_u32, NAME, OPT, 0, NULL)
>  #define fsparam_u32oct(NAME, OPT) \
>  			__fsparam(fs_param_is_u32, NAME, OPT, 0, (void *)8)


Reviewed-by: Bernd Schubert <bschubert@ddn.com>