[U-Boot] [PATCH] cmd: avoid decimal conversion

[Simon Goldschmidt]

On Thu, Oct 10, 2019 at 12:46 PM Michal Simek <michal.simek at xilinx.com> wrote:
>
> On 09. 10. 19 19:28, Simon Goldschmidt wrote:
> > Am 09.10.2019 um 18:26 schrieb Tom Rini:
> >> On Tue, Oct 08, 2019 at 10:48:39AM +0200, Michal Simek wrote:
> >>> Hi Tom,
> >>>
> >>> On 19. 09. 19 15:28, Michal Simek wrote:
> >>>> On 13. 09. 19 17:09, Tom Rini wrote:
> >>>>> On Wed, Sep 11, 2019 at 03:39:53PM +0200, Michal Simek wrote:
> >>>>>
> >>>>>> From: T Karthik Reddy <t.karthik.reddy at xilinx.com>
> >>>>>>
> >>>>>> This patch uses auto instead of decimal in simple_strtoul().
> >>>>>>
> >>>>>> Signed-off-by: T Karthik Reddy <t.karthik.reddy at xilinx.com>
> >>>>>> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
> >>>>>> ---
> >>>>>>
> >>>>>>   cmd/test.c | 24 ++++++++++++------------
> >>>>>>   1 file changed, 12 insertions(+), 12 deletions(-)
> >>>>>>
> >>>>>> diff --git a/cmd/test.c b/cmd/test.c
> >>>>>> index fa0c349f0827..258bfd880653 100644
> >>>>>> --- a/cmd/test.c
> >>>>>> +++ b/cmd/test.c
> >>>>>> @@ -113,28 +113,28 @@ static int do_test(cmd_tbl_t *cmdtp, int
> >>>>>> flag, int argc, char * const argv[])
> >>>>>>               expr = strcmp(ap[0], ap[2]) > 0;
> >>>>>>               break;
> >>>>>>           case OP_INT_EQ:
> >>>>>> -            expr = simple_strtol(ap[0], NULL, 10) ==
> >>>>>> -                    simple_strtol(ap[2], NULL, 10);
> >>>>>> +            expr = simple_strtol(ap[0], NULL, 0) ==
> >>>>>> +                    simple_strtol(ap[2], NULL, 0);
> >>>>>>               break;
> >>>>>>           case OP_INT_NEQ:
> >>>>>> -            expr = simple_strtol(ap[0], NULL, 10) !=
> >>>>>> -                    simple_strtol(ap[2], NULL, 10);
> >>>>>> +            expr = simple_strtol(ap[0], NULL, 0) !=
> >>>>>> +                    simple_strtol(ap[2], NULL, 0);
> >>>>>>               break;
> >>>>>>           case OP_INT_LT:
> >>>>>> -            expr = simple_strtol(ap[0], NULL, 10) <
> >>>>>> -                    simple_strtol(ap[2], NULL, 10);
> >>>>>> +            expr = simple_strtol(ap[0], NULL, 0) <
> >>>>>> +                    simple_strtol(ap[2], NULL, 0);
> >>>>>>               break;
> >>>>>>           case OP_INT_LE:
> >>>>>> -            expr = simple_strtol(ap[0], NULL, 10) <=
> >>>>>> -                    simple_strtol(ap[2], NULL, 10);
> >>>>>> +            expr = simple_strtol(ap[0], NULL, 0) <=
> >>>>>> +                    simple_strtol(ap[2], NULL, 0);
> >>>>>>               break;
> >>>>>>           case OP_INT_GT:
> >>>>>> -            expr = simple_strtol(ap[0], NULL, 10) >
> >>>>>> -                    simple_strtol(ap[2], NULL, 10);
> >>>>>> +            expr = simple_strtol(ap[0], NULL, 0) >
> >>>>>> +                    simple_strtol(ap[2], NULL, 0);
> >>>>>>               break;
> >>>>>>           case OP_INT_GE:
> >>>>>> -            expr = simple_strtol(ap[0], NULL, 10) >=
> >>>>>> -                    simple_strtol(ap[2], NULL, 10);
> >>>>>> +            expr = simple_strtol(ap[0], NULL, 0) >=
> >>>>>> +                    simple_strtol(ap[2], NULL, 0);
> >>>>>>               break;
> >>>>>>           case OP_FILE_EXISTS:
> >>>>>>               expr = file_exists(ap[1], ap[2], ap[3], FS_TYPE_ANY);
> >>>>>
> >>>>> I'm going to NAK this, but could be argued around to changing my mind.
> >>>>> While it's true that in general command inputs are hex and not
> >>>>> decimal,
> >>>>> this has been decimal since introduction in 2009.  So changing it
> >>>>> now is
> >>>>> breaking ABI and other peoples test scripts, so I don't think we
> >>>>> can do
> >>>>> this, sorry.
> >>>>
> >>>> I also think that this is not breaking any ABI. test_hush_if_test.py is
> >>>> around for a while to capture issues in this space and I can't see any
> >>>> single failure in connection to this change.
> >>>>
> >>>> If this accepted then we can add more tests like this
> >>>>      ('test 0x2000000 -gt 0x2000001', False),
> >>>>      ('test 0x2000000 -gt 0x2000000', False),
> >>>>      ('test 0x2000000 -gt 0x1ffffff', True),
> >>>>      ('test 2000000 -gt 0x1ffffff', False),
> >>>>      ('test 0x2000000 -gt 1ffffff', True),
> >>>>
> >>>>      ('test 0x2000000 -lt 1ffffff', False),
> >>>>      ('test 0x2000000 -eq 2000000', False),
> >>>>      ('test 0x2000000 -ne 2000000', True),
> >>>>
> >>>> where some of them are failing without this patch
> >>>> ... test_hush_if_test[test 0x2000000 -gt 0x1ffffff-True]
> >>>>
> >>>> ... test_hush_if_test[test 2000000 -gt 0x1ffffff-False]
> >>>>
> >>>> ... test_hush_if_test[test 0x2000000 -gt 1ffffff-True]
> >>>>
> >>>> ... test_hush_if_test[test 0x2000000 -lt 1ffffff-False]
> >>>>
> >>>
> >>> Any comment on this?
> >>
> >> Sorry, yes, OK, we can take this then.  I should have this in my first
> >> batch of non-python general changes I grab.
> >
> > But strtoul("010", NULL, 0) is 8 while strtoul("010", NULL, 10) is 10.
> > Do we care for that change?
>
> Leading zeroes are problematic
>
> Before the patch this pass
> test 010 -eq 10 && echo yes
>
> After the patch this fail.
>
> I can't see any test in test/py/tests/test_hush_if_test.py to cover this
> case that's why I can't guess if this is used or not.

Hmm, it might not be a problem then...?

Regards,
Simon