[PATCH] Revert "lib: Improve _parse_integer_fixup_radix base 16 detection"

[Simon Glass]

Hi,

On Mon, 8 Jun 2020 at 00:24, Michal Simek <michal.simek at xilinx.com> wrote:
>
> On 07. 06. 20 7:36, Sean Anderson wrote:
> > This reverts commit 0486497e2b5f4d36fa968a1a60fea358cbf70b65.
> >
> > The strtoul has well-defined semantics. It is defined by the C standard and
> > POSIX. To quote the relevant section of the man pages,
> >
> >> If base is zero or 16, the string may then include a "0x" prefix, and the
> >> number will be read in base 16; otherwise, a zero base is taken as 10
> >> (decimal) unless the next character is '0', in which case it is taken as
> >> 8 (octal).
> >
> > Keeping these semantics is important for several reasons. First, it is very
> > surprising for standard library functions to behave differently than usual.
> > Every other implementation of strtoul has different semantics than the
> > implementation in U-Boot at the moment. Second, it can result in very
> > surprising results from small changes. For example, changing the string
> > "1f" to "20" causes the parsed value to *decrease*. Forcing use of the "0x"
> > prefix to specify hexidecimal numbers is a feature, not a bug. Lastly, this
> > is slightly less performant, since the entire number is parsed twice.
> >
> > This fixes the str_simple_strtoul test failing with
> >
> > test/str_ut.c:29, run_strtoul(): expect_val == val: Expected 0x44b (1099), got 0x1099ab (1087915)
> > test/str_ut.c:46, str_simple_strtoul(): 0 == run_strtoul(uts, str2, 0, 1099, 4): Expected 0x0 (0), got 0x1 (1)
> >
> > Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> > CC: Michal Simek <michal.simek at xilinx.com>
> > CC: Shiril Tichkule <shirilt at xilinx.com>
> > ---
> >
> >  lib/strto.c | 18 +-----------------
> >  1 file changed, 1 insertion(+), 17 deletions(-)
> >
> > diff --git a/lib/strto.c b/lib/strto.c
> > index 3d77115d4d..c00bb5895d 100644
> > --- a/lib/strto.c
> > +++ b/lib/strto.c
> > @@ -22,25 +22,9 @@ static const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
> >                               *base = 16;
> >                       else
> >                               *base = 8;
> > -             } else {
> > -                     int i = 0;
> > -                     char var;
> > -
> > +             } else
> >                       *base = 10;
> > -
> > -                     do {
> > -                             var = tolower(s[i++]);
> > -                             if (var >= 'a' && var <= 'f') {
> > -                                     *base = 16;
> > -                                     break;
> > -                             }
> > -
> > -                             if (!(var >= '0' && var <= '9'))
> > -                                     break;
> > -                     } while (var);
> > -             }
> >       }
> > -
> >       if (*base == 16 && s[0] == '0' && tolower(s[1]) == 'x')
> >               s += 2;
> >       return s;
> >
>
>
> It is in u-boot mainline from February. Then we had to fix it in April.
> In the middle of this I have seen IIC one patchset which improves hex
> handling which is likely better way then this.
> I am fine with reverting this patch but I would also like to see more
> comments in the code to say that we shouldn't really change this.

Once this revert is in I think we should refrain from changing it
until there are tests to cover its current behaviour, e.g. in str_ut.c

Reviewed-by: Simon Glass <sjg at chromium.org>

Regards,
Simon