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

Andy Zhou azhou at nicira.com
Wed May 27 22:15:32 UTC 2015


On Wed, May 27, 2015 at 10:48 AM, Jesse Gross <jesse at nicira.com> wrote:
> Geneve options are variable length and up to 124 bytes long, which means
> that they can't be easily manipulated by the integer string functions
> like we do for other fields. This adds a few helper routines to make
> these operations easier.
>
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> ---

Looks good overall, I have some comments below. but they won't affect
the correctness.

Acked-by: Andy Zhou <azhou at nicira.com>



>  lib/dynamic-string.c | 24 +++++++++++++++
>  lib/dynamic-string.h |  1 +
>  lib/util.c           | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/util.h           |  3 ++
>  4 files changed, 111 insertions(+)
>
> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> index 914af64..ad6941d 100644
> --- a/lib/dynamic-string.c
> +++ b/lib/dynamic-string.c
> @@ -361,6 +361,30 @@ ds_swap(struct ds *a, struct ds *b)
>      *b = temp;
>  }
>
> +void
> +ds_put_hex(struct ds *ds, const void *buf_, size_t size)
> +{
> +    const uint8_t *buf = buf_;
> +    bool printed = false;
> +    int i;
> +
> +    ds_put_cstr(ds, "0x");
> +    for (i = 0; i < size; i++) {
> +        uint8_t val = buf[i];
> +        if (val || printed) {
> +            if (!printed) {
> +                ds_put_format(ds, "%"PRIx8, val);
> +            } else {
> +                ds_put_format(ds, "%02"PRIx8, val);
> +            }
> +            printed = true;
> +        }
> +    }
> +    if (!printed) {
> +        ds_put_char(ds, '0');
> +    }
> +}
> +
>  /* Writes the 'size' bytes in 'buf' to 'string' as hex bytes arranged 16 per
>   * line.  Numeric offsets are also included, starting at 'ofs' for the first
>   * byte in 'buf'.  If 'ascii' is true then the corresponding ASCII characters
> diff --git a/lib/dynamic-string.h b/lib/dynamic-string.h
> index dc5981a..95172d1 100644
> --- a/lib/dynamic-string.h
> +++ b/lib/dynamic-string.h
> @@ -55,6 +55,7 @@ void ds_put_format(struct ds *, const char *, ...) OVS_PRINTF_FORMAT(2, 3);
>  void ds_put_format_valist(struct ds *, const char *, va_list)
>      OVS_PRINTF_FORMAT(2, 0);
>  void ds_put_printable(struct ds *, const char *, size_t);
> +void ds_put_hex(struct ds *ds, const void *buf, size_t size);
>  void ds_put_hex_dump(struct ds *ds, const void *buf_, size_t size,
>                       uintptr_t ofs, bool ascii);
>  int ds_get_line(struct ds *, FILE *);
> diff --git a/lib/util.c b/lib/util.c
> index bcf7700..3dc06d0 100644
> --- a/lib/util.c
> +++ b/lib/util.c
> @@ -738,6 +738,89 @@ hexits_value(const char *s, size_t n, bool *ok)
>      return value;
>  }
>
> +/* Parses the string in 's' as an integer in either hex or decimal format and
> + * puts the result right justified in the array 'valuep' that is 'field_width'
> + * big. If the string is in hex format, the value may be arbitrarily large;
> + * integers are limited to 64-bit values. (The rationale is that decimal is
> + * likely to represent a number and 64 bits is a reasonable maximum whereas
> + * hex could either be a number or a byte string.)
> + *
> + * On return 'tail' points to the first character in the string that was
> + * not parsed as part of the value. ERANGE is returned if the value is too
> + * large to fit in the given field. */
> +int
> +parse_int_string(const char *s, uint8_t *valuep, int field_width, char **tail)
> +{
> +    unsigned long long int integer;
> +    int i;
> +
> +    if (ovs_scan(s, "0x")) {
> +        uint8_t *hexit_str;
> +        bool first_char = false;
> +        int len = 0;
> +        int val_idx;
> +        int err = 0;
> +
> +        s += 2;
> +        hexit_str = xmalloc(field_width * 2);
> +
> +        for (;;) {
> +            uint8_t hexit;
> +            bool ok;
> +
> +            s += strspn(s, " \t\r\n");
Should we allow space or '\t' in the middle of a number?
> +            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.
> +                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.
> +            val_idx--;
> +        }
> +
> +free:
> +        free(hexit_str);
> +       return err;
> +    }
> +
> +    errno = 0;
> +    integer = strtoull(s, tail, 0);
> +    if (errno) {
> +        return errno;
> +    }
> +
> +    for (i = field_width - 1; i >= 0; i--) {
> +        valuep[i] = integer;
> +        integer >>= 8;
> +    }
> +    if (integer) {
> +        return ERANGE;
> +    }
> +
> +    return 0;
> +}
> +
>  /* Returns the current working directory as a malloc()'d string, or a null
>   * pointer if the current working directory cannot be determined. */
>  char *
> diff --git a/lib/util.h b/lib/util.h
> index 276edb5..78abfd3 100644
> --- a/lib/util.h
> +++ b/lib/util.h
> @@ -314,6 +314,9 @@ bool str_to_double(const char *, double *);
>  int hexit_value(int c);
>  uintmax_t hexits_value(const char *s, size_t n, bool *ok);
>
> +int parse_int_string(const char *s, uint8_t *valuep, int field_width,
> +                     char **tail);
> +
>  const char *english_list_delimiter(size_t index, size_t total);
>
>  char *get_cwd(void);
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list