On Wed, May 27, 2015 at 10:48 AM, Jesse Gross <je...@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 <je...@nicira.com>
> ---
Looks good overall, I have some comments below. but they won't affect
the correctness.
Acked-by: Andy Zhou <az...@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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
