[ovs-dev] [PATCH 3/4] util: Library routines for printing and scanning large hex integers.

[Jesse Gross]

On Wed, May 27, 2015 at 3:15 PM, Andy Zhou <azhou at nicira.com> wrote:
> On Wed, May 27, 2015 at 10:48 AM, Jesse Gross <jesse at nicira.com> wrote:
>> diff --git a/lib/util.c b/lib/util.c
>> index bcf7700..3dc06d0 100644
>> --- a/lib/util.c
>> +++ b/lib/util.c
>> +int
>> +parse_int_string(const char *s, uint8_t *valuep, int field_width, char **tail)
>> +{
[...]
>> +            s += strspn(s, " \t\r\n");
> Should we allow space or '\t' in the middle of a number?

This is to handle the style of writing hex grouped as pairs of bytes
with spaces in between, so I think it is nice to support it.

>> +            hexit = hexits_value(s, 1, &ok);
>> +            if (!ok) {
>> +                *tail = CONST_CAST(char *, s);
>> +                break;
>> +            }
>> +
>> +            if (hexit != 0 || first_char) {
> Is checking for 'len' sufficient here? May be we can avoid introducing
> the 'first_char' variable.

Yes, I think that works and simplifies things.

>> +                if (DIV_ROUND_UP(len + 1, 2) > field_width) {
>> +                    err = ERANGE;
>> +                    goto free;
>> +                }
>> +
>> +                hexit_str[len] = hexit;
>> +                len++;
>> +                first_char = true;
>> +            }
>> +            s++;
>> +        }
>> +
>> +        memset(valuep, 0, field_width);
> we can probably move memset after the for loop in case most digits are
> covered by the conversion.
[...]
>> +
>> +        val_idx = field_width - 1;
>> +        for (i = len - 1; i >= 0; i -= 2) {
>> +            if (i > 0) {
>> +                valuep[val_idx] = hexit_str[i - 1] << 4;
>> +            }
>> +            valuep[val_idx] += hexit_str[i];
> If we switch the order of adding lower 4 bits and high 4 bits, then we
> don't need to set valuep[val_idx] to zero beforehand.

Sure, both of those seem fine as a small optimization.