diff mbox series

[v2] lib: Improve _parse_integer_fixup_radix base 16 detection

Message ID 3435e8a9a13f8ae6b68c2264ed787293a4fa483e.1583138185.git.michal.simek@xilinx.com
State Superseded
Delegated to: Tom Rini
Headers show
Series [v2] lib: Improve _parse_integer_fixup_radix base 16 detection | expand

Commit Message

Michal Simek March 2, 2020, 8:36 a.m. UTC 
Base autodetection is failing for this case:
if test 257 -gt 3ae; then echo first; else echo second; fi

It is because base for 3ae is recognized by _parse_integer_fixup_radix() as
10. The patch is checking all chars to make sure that they are not 'a' or
up. If they are base needs to be in hex.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Shiril Tichkule <shirilt@xlinx.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---

Changes in v2:
- Fix end of string to be \0 instead of \n
- Detect hex by checking chars between a-f to avoid cases like number
  follow by ;

 lib/strto.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko March 3, 2020, 9:37 a.m. UTC | #1 
On Mon, Mar 2, 2020 at 10:36 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
> Base autodetection is failing for this case:
> if test 257 -gt 3ae; then echo first; else echo second; fi
>
> It is because base for 3ae is recognized by _parse_integer_fixup_radix() as
> 10. The patch is checking all chars to make sure that they are not 'a' or
> up. If they are base needs to be in hex.

...

> +                       for (i = 0; ; i++) {
> +                               char var = s[i];
> +
> +                               if (var == '\0')
> +                                       break;
> +
> +                               if ((var >= 'a' && var <= 'f') ||
> +                                   (var >= 'A' && var <= 'F')) {
> +                                       *base = 16;
> +                                       break;
> +                               }
> +                       }

int i = 0;
char var;

do {
  var = tolower(s[i++]);
  if (...) {
   *base = 16;
   break;
  }
} while (var);

or alike?

...

>         }

> +

Is this relevant?

>         if (*base == 16 && s[0] == '0' && tolower(s[1]) == 'x')
>                 s += 2;
>         return s;
Michal Simek March 3, 2020, 12:02 p.m. UTC | #2 
On 03. 03. 20 10:37, Andy Shevchenko wrote:
> On Mon, Mar 2, 2020 at 10:36 AM Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> Base autodetection is failing for this case:
>> if test 257 -gt 3ae; then echo first; else echo second; fi
>>
>> It is because base for 3ae is recognized by _parse_integer_fixup_radix() as
>> 10. The patch is checking all chars to make sure that they are not 'a' or
>> up. If they are base needs to be in hex.
> 
> ...
> 
>> +                       for (i = 0; ; i++) {
>> +                               char var = s[i];
>> +
>> +                               if (var == '\0')
>> +                                       break;
>> +
>> +                               if ((var >= 'a' && var <= 'f') ||
>> +                                   (var >= 'A' && var <= 'F')) {
>> +                                       *base = 16;
>> +                                       break;
>> +                               }
>> +                       }
> 
> int i = 0;
> char var;
> 
> do {
>   var = tolower(s[i++]);
>   if (...) {
>    *base = 16;
>    break;
>   }
> } while (var);
> 
> or alike?

not a problem with this.


> 
> ...
> 
>>         }
> 
>> +
> 
> Is this relevant?

it can be removed from this patch but I can't see any issue with it to
making code more readable.

> 
>>         if (*base == 16 && s[0] == '0' && tolower(s[1]) == 'x')
>>                 s += 2;
>>         return s;
> 

M
Andy Shevchenko March 3, 2020, 2:52 p.m. UTC | #3 
On Tue, Mar 3, 2020 at 2:03 PM Michal Simek <michal.simek@xilinx.com> wrote:
> On 03. 03. 20 10:37, Andy Shevchenko wrote:
> > On Mon, Mar 2, 2020 at 10:36 AM Michal Simek <michal.simek@xilinx.com> wrote:

> >> Base autodetection is failing for this case:
> >> if test 257 -gt 3ae; then echo first; else echo second; fi
> >>
> >> It is because base for 3ae is recognized by _parse_integer_fixup_radix() as
> >> 10.

>>> The patch is checking all chars to make sure that they are not 'a' or
> >> up. If they are base needs to be in hex.

Hold on!

> >> +                       for (i = 0; ; i++) {
> >> +                               char var = s[i];
> >> +
> >> +                               if (var == '\0')
> >> +                                       break;
> >> +
> >> +                               if ((var >= 'a' && var <= 'f') ||
> >> +                                   (var >= 'A' && var <= 'F')) {
> >> +                                       *base = 16;
> >> +                                       break;
> >> +                               }
> >> +                       }

Actually your code and commit message are unaligned. In the code you
stop on the *first* hex character without checking the rest.
Michal Simek March 3, 2020, 2:59 p.m. UTC | #4 
On 03. 03. 20 15:52, Andy Shevchenko wrote:
> On Tue, Mar 3, 2020 at 2:03 PM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 03. 03. 20 10:37, Andy Shevchenko wrote:
>>> On Mon, Mar 2, 2020 at 10:36 AM Michal Simek <michal.simek@xilinx.com> wrote:
> 
>>>> Base autodetection is failing for this case:
>>>> if test 257 -gt 3ae; then echo first; else echo second; fi
>>>>
>>>> It is because base for 3ae is recognized by _parse_integer_fixup_radix() as
>>>> 10.
> 
>>>> The patch is checking all chars to make sure that they are not 'a' or
>>>> up. If they are base needs to be in hex.
> 
> Hold on!
> 
>>>> +                       for (i = 0; ; i++) {
>>>> +                               char var = s[i];
>>>> +
>>>> +                               if (var == '\0')
>>>> +                                       break;
>>>> +
>>>> +                               if ((var >= 'a' && var <= 'f') ||
>>>> +                                   (var >= 'A' && var <= 'F')) {
>>>> +                                       *base = 16;
>>>> +                                       break;
>>>> +                               }
>>>> +                       }
> 
> Actually your code and commit message are unaligned. In the code you
> stop on the *first* hex character without checking the rest.

That's correct because base is detected as 10 at this stage.

The question is if make sense to check all chars because if there is one
char between a<= <=f then you are sure that it is not base 10.

And if you want to check all of them then you need to also detect chars
like ';' which can be there as you see above.

Thanks,
Michal
diff mbox series

Patch

diff --git a/lib/strto.c b/lib/strto.c
index 55ff9f7437d5..4ac79f46a5e2 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -22,9 +22,26 @@  static const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
 				*base = 16;
 			else
 				*base = 8;
-		} else
+		} else {
+			int i;
+
 			*base = 10;
+
+			for (i = 0; ; i++) {
+				char var = s[i];
+
+				if (var == '\0')
+					break;
+
+				if ((var >= 'a' && var <= 'f') ||
+				    (var >= 'A' && var <= 'F')) {
+					*base = 16;
+					break;
+				}
+			}
+		}
 	}
+
 	if (*base == 16 && s[0] == '0' && tolower(s[1]) == 'x')
 		s += 2;
 	return s;