Re: [PATCH] btrfs: manage thread_pool mount option as %u

[David Sterba <dsterba@suse.cz>]

On Tue, Feb 13, 2018 at 05:34:56PM +0200, Nikolay Borisov wrote:
> >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> >>> index 02c7766e6849..8112619cac95 100644
> >>> --- a/fs/btrfs/super.c
> >>> +++ b/fs/btrfs/super.c
> >>> @@ -346,7 +346,7 @@ static const match_table_t tokens = {
> >>>       {Opt_barrier, "barrier"},
> >>>       {Opt_max_inline, "max_inline=%u"},
> >>>       {Opt_alloc_start, "alloc_start=%s"},
> >>> -    {Opt_thread_pool, "thread_pool=%d"},
> >>> +    {Opt_thread_pool, "thread_pool=%u"},
> >>>       {Opt_compress, "compress"},
> >>>       {Opt_compress_type, "compress=%s"},
> >>>       {Opt_compress_force, "compress-force"},
> >>> @@ -596,12 +596,11 @@ int btrfs_parse_options(struct btrfs_fs_info
> >>> *info, char *options,
> >>>               ret = match_int(&args[0], &intarg);
> >>>               if (ret) {
> >>>                   goto out;
> >>> -            } else if (intarg > 0) {
> >>> -                info->thread_pool_size = intarg;
> >>> -            } else {
> >>> +            } else if (intarg == 0) {
> >>
> >> One thing I'm worried about is the fact that match_int parses a signed
> >> int. So If someone pases -1 then it would be parsed to -1 but when you
> >> set it to thread_pool_size the actual value is going to be the 2's
> >> complement of the value i.e. a very large number. So a check for intarg
> >> < 0 is required to avoid that. Same applies to your other patches
> > 
> >  That's not true. When -o thread_pool=-1 is passed it would fail
> >  to match to any token. And same applies to other patches too.
> 
> Indeed, the subtleties of the matching machinery. In that case for the
> whole series:

Yeah, it's not very clear from the sources, but with %u the parser
refuses negative numbers. Even with match_int, the typecasts will not
damage the result when assigning to u32 from intarg.