On Tue, Oct 29, 2024 at 1:58 PM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 10/11/24 21:13, Joanne Koong wrote:
> > Add a fsparam helper for unsigned 16 bit values.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > 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()?
Thanks for taking a look at this patchset, Bernd!
I'm not sure what the correct answer here is either with whether this
should follow fs_param_is_u32() or fs_param_is_u64(). I leaned towards
fs_param_is_u32() because that seemed more permissive in supporting
octal/hex.
I see in the commit history (commit 328de5287b10a) that Al Viro added
both of these. After the fuse parts of this patchset gets feedback
from Miklos, I'll cc Al on the next iteration and hopefully that
should give us some more clarity.
Thanks,
Joanne
>
> > 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@xxxxxxx>
