[ovs-dev] [PATCH] metaflow: Convert hex parsing to use new utility functions.
Andy Zhou
azhou at nicira.com
Tue Jun 2 01:54:18 UTC 2015
I have a minor comments in line.
Overall, looks good. Acked-by: Andy Zhou <azhou at nicira.com>
On Mon, Jun 1, 2015 at 11:34 AM, Jesse Gross <jesse at nicira.com> wrote:
> We now have functions that can do parsing and printing of long hex
> strings, so we should use them for meta flow fields to ensure
> consistent behavior.
>
> Since these functions can handle infinitely long strings, we can
> also increase the maximum field size for MFS_HEXADECIMAL types to
> the limit allowed by NXM/OXM. This is useful for future large fields,
> such as Geneve options.
>
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> ---
> build-aux/extract-ofp-fields | 20 +++++++--------
> lib/meta-flow.c | 59 +++++++++++++++++---------------------------
> 2 files changed, 32 insertions(+), 47 deletions(-)
>
> diff --git a/build-aux/extract-ofp-fields b/build-aux/extract-ofp-fields
> index 315552d..ca2ca04 100755
> --- a/build-aux/extract-ofp-fields
> +++ b/build-aux/extract-ofp-fields
> @@ -21,16 +21,16 @@ TYPES = {"u8": 1,
> "be64": 8,
> "IPv6": 16}
>
> -FORMATTING = {"decimal": ("MFS_DECIMAL", 1, 8),
> - "hexadecimal": ("MFS_HEXADECIMAL", 1, 8),
> - "Ethernet": ("MFS_ETHERNET", 6, 6),
> - "IPv4": ("MFS_IPV4", 4, 4),
> - "IPv6": ("MFS_IPV6", 16, 16),
> - "OpenFlow 1.0 port": ("MFS_OFP_PORT", 2, 2),
> - "OpenFlow 1.1+ port": ("MFS_OFP_PORT_OXM", 4, 4),
> - "frag": ("MFS_FRAG", 1, 1),
> - "tunnel flags": ("MFS_TNL_FLAGS", 2, 2),
> - "TCP flags": ("MFS_TCP_FLAGS", 2, 2)}
> +FORMATTING = {"decimal": ("MFS_DECIMAL", 1, 8),
> + "hexadecimal": ("MFS_HEXADECIMAL", 1, 127),
> + "Ethernet": ("MFS_ETHERNET", 6, 6),
> + "IPv4": ("MFS_IPV4", 4, 4),
> + "IPv6": ("MFS_IPV6", 16, 16),
> + "OpenFlow 1.0 port": ("MFS_OFP_PORT", 2, 2),
> + "OpenFlow 1.1+ port": ("MFS_OFP_PORT_OXM", 4, 4),
> + "frag": ("MFS_FRAG", 1, 1),
> + "tunnel flags": ("MFS_TNL_FLAGS", 2, 2),
> + "TCP flags": ("MFS_TCP_FLAGS", 2, 2)}
>
> PREREQS = {"none": "MFP_NONE",
> "ARP": "MFP_ARP",
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 757843d..28d5931 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1684,39 +1684,32 @@ static char *
> mf_from_integer_string(const struct mf_field *mf, const char *s,
> uint8_t *valuep, uint8_t *maskp)
> {
> - unsigned long long int integer, mask;
> char *tail;
> - int i;
> + int err;
>
> - errno = 0;
> - integer = strtoull(s, &tail, 0);
> - if (errno || (*tail != '\0' && *tail != '/')) {
> + err = parse_int_string(s, valuep, mf->n_bytes, &tail);
> + if (err || (*tail != '\0' && *tail != '/')) {
> goto syntax_error;
> }
>
> if (*tail == '/') {
> - mask = strtoull(tail + 1, &tail, 0);
> - if (errno || *tail != '\0') {
> + err = parse_int_string(tail + 1, maskp, mf->n_bytes, &tail);
> + if (err || *tail != '\0') {
> goto syntax_error;
> }
> } else {
> - mask = ULLONG_MAX;
> + memset(maskp, 0xff, mf->n_bytes);
> }
>
> - for (i = mf->n_bytes - 1; i >= 0; i--) {
> - valuep[i] = integer;
> - maskp[i] = mask;
> - integer >>= 8;
> - mask >>= 8;
> - }
> - if (integer) {
> - return xasprintf("%s: value too large for %u-byte field %s",
> - s, mf->n_bytes, mf->name);
> - }
> return NULL;
>
> syntax_error:
> - return xasprintf("%s: bad syntax for %s", s, mf->name);
> + if (err == ERANGE) {
> + return xasprintf("%s: value too large for %u-byte field %s",
> + s, mf->n_bytes, mf->name);
This error message can be more helpful: When it shows up, it is not
clear it is value or mask part caused the error. When it is integer
output range,
the message talked about number of bytes that can be confusing.
On the other hand, giving extra information may be overkill here, and
it is close
to what current code does. So it would be nice to improve, but not a must.
> + } else {
> + return xasprintf("%s: bad syntax for %s", s, mf->name);
> + }
> }
>
> static char *
> @@ -2111,33 +2104,25 @@ static void
> mf_format_integer_string(const struct mf_field *mf, const uint8_t *valuep,
> const uint8_t *maskp, struct ds *s)
> {
> - unsigned long long int integer;
> - int i;
> -
> - ovs_assert(mf->n_bytes <= 8);
> -
> - integer = 0;
> - for (i = 0; i < mf->n_bytes; i++) {
> - integer = (integer << 8) | valuep[i];
> - }
> if (mf->string == MFS_HEXADECIMAL) {
> - ds_put_format(s, "%#llx", integer);
> + ds_put_hex(s, valuep, mf->n_bytes);
> } else {
> - ds_put_format(s, "%lld", integer);
> - }
> + unsigned long long int integer = 0;
> + int i;
>
> - if (maskp) {
> - unsigned long long int mask;
> -
> - mask = 0;
> + ovs_assert(mf->n_bytes <= 8);
> for (i = 0; i < mf->n_bytes; i++) {
> - mask = (mask << 8) | maskp[i];
> + integer = (integer << 8) | valuep[i];
> }
> + ds_put_format(s, "%lld", integer);
> + }
>
> + if (maskp) {
> /* I guess we could write the mask in decimal for MFS_DECIMAL but I'm
> * not sure that that a bit-mask written in decimal is ever easier to
> * understand than the same bit-mask written in hexadecimal. */
> - ds_put_format(s, "/%#llx", mask);
> + ds_put_char(s, '/');
> + ds_put_hex(s, maskp, mf->n_bytes);
> }
> }
>
> --
> 2.1.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list