| Message ID | 537B16CB.9070508@huawei.com |
|---|---|
| State | Rejected |
| Headers | show |
On 05/20/2014 11:48 AM, Zhang Zhen wrote: > use the newer and more pleasant kstrtoint() to replace simple_strtoul(), > because simple_strtoul() is marked for obsoletion. > > Signed-off-by: Zhang Zhen <zhenzhang.zhang@huawei.com> > Signed-off-by: hujianyang <hujianyang@huawei.com> > --- > drivers/mtd/ubi/build.c | 7 +++---- > fs/ubifs/super.c | 7 ++++--- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c > index 6e30a3c..77a2884 100644 > --- a/drivers/mtd/ubi/build.c > +++ b/drivers/mtd/ubi/build.c > @@ -1190,11 +1190,10 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) > static struct mtd_info * __init open_mtd_device(const char *mtd_dev) > { > struct mtd_info *mtd; > - int mtd_num; > - char *endp; > + int mtd_num, ret; > > - mtd_num = simple_strtoul(mtd_dev, &endp, 0); > - if (*endp != '\0' || mtd_dev == endp) { > + ret = kstrtoint(mtd_dev, 0, &mtd_num); > + if (ret) { > /* > * This does not look like an ASCII integer, probably this is > * MTD device name. > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index a81c7b5..d8c71a6 100644 > --- 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; > + ret = kstrtoint(endptr, 0, &dev); But endptr is used in the code later, so this is wrong. > > /* ubiY method */ > - if (*endptr == '\0') > + if (!ret) > return ubi_open_volume(0, dev, mode); > > /* ubiX_Y method */ >
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: 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
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 > >
On Tue, May 20, 2014 at 2:45 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> + 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. Sorry, you're right. Thanks for noticing! 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
On 2014/5/20 20:55, Geert Uytterhoeven wrote: > On Tue, May 20, 2014 at 2:45 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> + 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. Sorry, i made a mistake. As parsing the number failed, it should just return ERR_PTR(-EINVAL). Thank you for your review. I learned a lot from you and Geert. I will send another version. > > Sorry, you're right. Thanks for noticing! > > 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 > >
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c index 6e30a3c..77a2884 100644 --- a/drivers/mtd/ubi/build.c +++ b/drivers/mtd/ubi/build.c @@ -1190,11 +1190,10 @@ static struct mtd_info * __init open_mtd_by_chdev(const char *mtd_dev) static struct mtd_info * __init open_mtd_device(const char *mtd_dev) { struct mtd_info *mtd; - int mtd_num; - char *endp; + int mtd_num, ret; - mtd_num = simple_strtoul(mtd_dev, &endp, 0); - if (*endp != '\0' || mtd_dev == endp) { + ret = kstrtoint(mtd_dev, 0, &mtd_num); + if (ret) { /* * This does not look like an ASCII integer, probably this is * MTD device name. diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index a81c7b5..d8c71a6 100644 --- 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; + ret = kstrtoint(endptr, 0, &dev); /* ubiY method */ - if (*endptr == '\0') + if (!ret) return ubi_open_volume(0, dev, mode); /* ubiX_Y method */