On Wed, May 27, 2015 at 3:15 PM, Andy Zhou <az...@nicira.com> wrote:
> On Wed, May 27, 2015 at 10:48 AM, Jesse Gross <je...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
