Re: [PATCH v2] UBIFS: replace simple_strtoul() with kstrtoint()

[Adrian Hunter <adrian.hunter@intel.com>]

On 05/20/2014 03:31 PM, Geert Uytterhoeven wrote:
> On Tue, May 20, 2014 at 2:21 PM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 05/20/2014 11:48 AM, Zhang Zhen wrote:
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -1903,7 +1903,7 @@ const struct super_operations ubifs_super_operations = {
>>>  static struct ubi_volume_desc *open_ubi(const char *name, int mode)
>>>  {
>>>       struct ubi_volume_desc *ubi;
>>> -     int dev, vol;
>>> +     int dev, vol, ret;
>>>       char *endptr;
>>>
>>>       /* First, try to open using the device node path method */
>>> @@ -1922,10 +1922,11 @@ static struct ubi_volume_desc *open_ubi(const char *name, int mode)
>>>       if (!isdigit(name[3]))
>>>               return ERR_PTR(-EINVAL);
>>>
>>> -     dev = simple_strtoul(name + 3, &endptr, 0);
>>> +     endptr = (char *)name + 3;
> 
> Please make endptr const char *, so the cast can be killed.
> Always think twice before adding a cast!
> 
> Ah, there's still another simple_strtoul() left below, that needs to be
> converted first.
> 
>>> +     ret = kstrtoint(endptr, 0, &dev);
>>
>> But endptr is used in the code later, so this is wrong.
> 
> That was my first thought, too. But upon closer look, I think it's correct.
> 
>>>
>>>       /* ubiY method */
>>> -     if (*endptr == '\0')
> 
> endptr would point to the trailing nul on success...
> 
>>> +     if (!ret)
> 
> ... which is now replaced by a test for ret not being an errorcoe.
> 
>>>               return ubi_open_volume(0, dev, mode);
>>>
>>>       /* ubiX_Y method */
> 
> If ret is an errorcode, the flow continues.
> 
> As parsing the number failed, the code checks if the first character
> (name + 3) is an underscore or colon:

No, it does not check if parsing failed, it checks for end-of-input.
The "X" and "Y" of "ubiX_Y" are numbers.

> 
>        if (*endptr == '_' && isdigit(endptr[1])) {
> 
> 
>                 vol = simple_strtoul(endptr + 1, &endptr, 0);
>                 if (*endptr != '\0')
>                         return ERR_PTR(-EINVAL);
>                 return ubi_open_volume(dev, vol, mode);
>         }
> 
>         /* ubiX:NAME method */
>         if ((*endptr == ':' || *endptr == '!') && endptr[1] != '\0')
>                 return ubi_open_volume_nm(dev, ++endptr, mode);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 
>