[ovs-dev] [ext-260 v2 4/6] ofp-errors: Implement OpenFlow 1.2+ experimenter error codes.

Reid Price rprice at nicira.com
Wed Feb 13 23:23:39 UTC 2013


Python seems good, nice use of int(X, 0)

One question on

> +                print "        {       -1, -1,  -1 }, /* %s */" % enum

If you moved the print outside this and used the same print (below) for both

> +                print "        { %#8x, %2d, %3d }, /* %s */" % (vendor, type_, code, enum)

this would've looked like -0x1.  If the else just did

  vendor = type_ = code = -1

in the else, seems easy to read/maintain

On Wed, Feb 13, 2013 at 2:32 PM, Ben Pfaff <blp at nicira.com> wrote:
> OpenFlow 1.2 standardized experimenter error codes in a way different from
> the Nicira extension.  This commit implements the OpenFlow 1.2+ version.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  build-aux/extract-ofp-errors    |  149 +++++++++++++++++++++++----------------
>  include/openflow/openflow-1.2.h |    5 +-
>  lib/ofp-errors.c                |   72 +++++++++++++------
>  lib/ofp-errors.h                |   75 +++++++++++---------
>  tests/ofp-errors.at             |   78 ++++++++++++++++++---
>  utilities/ovs-ofctl.c           |   15 +++--
>  6 files changed, 258 insertions(+), 136 deletions(-)
>
> diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
> index aa1f4b7..375876b 100755
> --- a/build-aux/extract-ofp-errors
> +++ b/build-aux/extract-ofp-errors
> @@ -151,10 +151,10 @@ def extract_ofp_errors(filenames):
>      names = []
>      domain = {}
>      reverse = {}
> -    for domain_name in ("OF1.0", "OF1.1", "OF1.2", "OF1.3",
> -                        "NX1.0", "NX1.1", "NX1.2", "NX1.3"):
> +    for domain_name in ("1.0", "1.1", "1.2", "1.3"):
>          domain[domain_name] = {}
>          reverse[domain_name] = {}
> +    vendor_map = {"OF": 0}
>
>      n_errors = 0
>      expected_errors = {}
> @@ -194,6 +194,11 @@ def extract_ofp_errors(filenames):
>                  expected_errors[m.group(1)] = (fileName, lineNumber)
>                  continue
>
> +            m = re.match('Experimenter\s+([A-Z]+)\s+(?:\([^)]*\)\s+)?=\s+([x0-9a-fA-F]+)\.$', comment)
> +            if m:
> +                vendor_map[m.group(1)] = int(m.group(2), 0)
> +                continue
> +
>              m = re.match('((?:.(?!\.  ))+.)\.  (.*)$', comment)
>              if not m:
>                  fatal("unexpected syntax between errors")
> @@ -201,7 +206,7 @@ def extract_ofp_errors(filenames):
>              dsts, comment = m.groups()
>
>              getLine()
> -            m = re.match('\s+(?:OFPERR_((?:OFP|NX)[A-Z0-9_]+))(\s*=\s*OFPERR_OFS)?,',
> +            m = re.match('\s+(?:OFPERR_([A-Z0-9_]+))(\s*=\s*OFPERR_OFS)?,',
>                           line)
>              if not m:
>                  fatal("syntax error expecting enum value")
> @@ -212,65 +217,82 @@ def extract_ofp_errors(filenames):
>              names.append(enum)
>
>              for dst in dsts.split(', '):
> -                m = re.match(r'([A-Z0-9.+]+)\((\d+)(?:,(\d+))?\)$', dst)
> +                m = re.match(r'([A-Z]+)([-+.0-9]*)\((\d+)(?:,(\d+))?\)$', dst)
>                  if not m:
>                      fatal("%s: syntax error in destination" % dst)
> -                targets = m.group(1)
> -                type_ = int(m.group(2))
> -                if m.group(3):
> -                    code = int(m.group(3))
> +                vendor_s = m.group(1)
> +                version_s = m.group(2)
> +                type_ = int(m.group(3))
> +                if m.group(4):
> +                    code = int(m.group(4))
>                  else:
>                      code = None
>
> -                target_map = {"OF1.0+": ("OF1.0", "OF1.1", "OF1.2", "OF1.3"),
> -                              "OF1.1+": ("OF1.1", "OF1.2", "OF1.3"),
> -                              "OF1.2+": ("OF1.2", "OF1.3"),
> -                              "OF1.3+": ("OF1.3",),
> -                              "OF1.0":  ("OF1.0",),
> -                              "OF1.1":  ("OF1.1",),
> -                              "OF1.2":  ("OF1.2",),
> -                              "OF1.3":  ("OF1.3",),
> -                              "NX1.0+": ("OF1.0", "OF1.1", "OF1.2", "OF1.3"),
> -                              "NX1.1+": ("OF1.1", "OF1.2", "OF1.3"),
> -                              "NX1.2+": ("OF1.2", "OF1.3"),
> -                              "NX1.3+": ("OF1.3",),
> -                              "NX1.0":  ("OF1.0",),
> -                              "NX1.1":  ("OF1.1",),
> -                              "NX1.2":  ("OF1.2",),
> -                              "NX1.3":  ("OF1.3",)}
> -                if targets not in target_map:
> -                    fatal("%s: unknown error domain" % targets)
> -                if targets.startswith('NX') and code < 0x100:
> -                    fatal("%s: NX domain code cannot be less than 0x100" % dst)
> -                if targets.startswith('OF') and code >= 0x100:
> -                    fatal("%s: OF domain code cannot be greater than 0x100"
> -                          % dst)
> -                for target in target_map[targets]:
> -                    domain[target].setdefault(type_, {})
> -                    if code in domain[target][type_]:
> -                        msg = "%d,%d in %s means both %s and %s" % (
> -                            type_, code, target,
> -                            domain[target][type_][code][0], enum)
> +                version_map = {"":        ("1.0", "1.1", "1.2", "1.3"),
> +                               "1.0+":    ("1.0", "1.1", "1.2", "1.3"),
> +                               "1.1+":    ("1.1", "1.2", "1.3"),
> +                               "1.2+":    ("1.2", "1.3"),
> +                               "1.3+":    ("1.3",),
> +                               "1.0":     ("1.0",),
> +                               "1.1":     ("1.1",),
> +                               "1.2":     ("1.2",),
> +                               "1.3":     ("1.3",),
> +                               "1.0-1.1": ("1.0", "1.1")}
> +                if version_s not in version_map:
> +                    fatal("%s: unknown version(s) %s" % (dst, version_s))
> +                versions = version_map[version_s]
> +
> +                if vendor_s not in vendor_map:
> +                    fatal("%s: unknown vendor %s" % (dst, vendor_s))
> +                vendor = vendor_map[vendor_s]
> +                if vendor == 0:
> +                    if code is None:
> +                        fatal("%s: both type and code are required" % dst)
> +                else:
> +                    pre12 = '1.0' in versions or '1.1' in versions
> +                    post12 = set(versions) - set(['1.0', '1.1']) != set()
> +                    if post12 and code is not None:
> +                        fatal("%s: OF1.2+ experimenter errors do not "
> +                              "have codes" % dst)
> +
> +                for version in versions:
> +                    if code is None:
> +                        if version in ('1.0', '1.1'):
> +                            t = 65535
> +                            c = type_
> +                        else:
> +                            t = type_
> +                            c = 0
> +                    else:
> +                        t = type_
> +                        c = code
> +                    domain[version].setdefault(vendor, {})
> +                    domain[version][vendor].setdefault(t, {})
> +                    if c in domain[version][vendor][t]:
> +                        msg = "%#x,%d,%d in OF%s means both %s and %s" % (
> +                            vendor, t, c, version,
> +                            domain[version][vendor][t][c][0], enum)
>                          if msg in expected_errors:
>                              del expected_errors[msg]
>                          else:
>                              error("%s: %s." % (dst, msg))
>                              sys.stderr.write("%s:%d: %s: Here is the location "
>                                               "of the previous definition.\n"
> -                                             % (domain[target][type_][code][1],
> -                                                domain[target][type_][code][2],
> +                                             % (domain[version][vendor][t][c][1],
> +                                                domain[version][vendor][t][c][2],
>                                                  dst))
>                      else:
> -                        domain[target][type_][code] = (enum, fileName,
> +                        domain[version][vendor][t][c] = (enum, fileName,
>                                                         lineNumber)
>
> -                    if enum in reverse[target]:
> -                        error("%s: %s in %s means both %d,%d and %d,%d." %
> -                              (dst, enum, target,
> -                               reverse[target][enum][0],
> -                               reverse[target][enum][1],
> -                               type_, code))
> -                    reverse[target][enum] = (type_, code)
> +                    if enum in reverse[version]:
> +                        error("%s: %s in OF%s means both %#x,%d,%d and %#x,%d,%d." %
> +                              (dst, enum, version,
> +                               reverse[version][enum][0],
> +                               reverse[version][enum][1],
> +                               reverse[version][enum][2],
> +                               vendor, t, c))
> +                    reverse[version][enum] = (vendor, t, c)
>
>          inputFile.close()
>
> @@ -289,8 +311,8 @@ def extract_ofp_errors(filenames):
>  struct ofperr_domain {
>      const char *name;
>      uint8_t version;
> -    enum ofperr (*decode)(uint16_t type, uint16_t code);
> -    struct pair errors[OFPERR_N_ERRORS];
> +    enum ofperr (*decode)(uint32_t vendor, uint16_t type, uint16_t code);
> +    struct triplet errors[OFPERR_N_ERRORS];
>  };
>
>  static const char *error_names[OFPERR_N_ERRORS] = {
> @@ -308,21 +330,26 @@ static const char *error_comments[OFPERR_N_ERRORS] = {
>      def output_domain(map, name, description, version):
>          print """
>  static enum ofperr
> -%s_decode(uint16_t type, uint16_t code)
> +%s_decode(uint32_t vendor, uint16_t type, uint16_t code)
>  {
> -    switch ((type << 16) | code) {""" % name
> +    switch (((uint64_t) vendor << 32) | (type << 16) | code) {""" % name
>          found = set()
>          for enum in names:
>              if enum not in map:
>                  continue
> -            type_, code = map[enum]
> +            vendor, type_, code = map[enum]
>              if code is None:
>                  continue
> -            value = (type_ << 16) | code
> +            value = (vendor << 32) | (type_ << 16) | code
>              if value in found:
>                  continue
>              found.add(value)
> -            print "    case (%d << 16) | %d:" % (type_, code)
> +            if vendor:
> +                vendor_s = "(%#xULL << 32) | " % vendor
> +            else:
> +                vendor_s = ""
> +            print("    case %s(%d << 16) | %d:"
> +                  % (vendor_s, type_, code))
>              print "        return OFPERR_%s;" % enum
>          print """\
>      }
> @@ -338,20 +365,20 @@ static const struct ofperr_domain %s = {
>      {""" % (name, description, version, name)
>          for enum in names:
>              if enum in map:
> -                type_, code = map[enum]
> +                vendor, type_, code = map[enum]
>                  if code == None:
>                      code = -1
> +                print "        { %#8x, %2d, %3d }, /* %s */" % (vendor, type_, code, enum)
>              else:
> -                type_ = code = -1
> -            print "        { %2d, %3d }, /* %s */" % (type_, code, enum)
> +                print "        {       -1, -1,  -1 }, /* %s */" % enum
>          print """\
>      },
>  };"""
>
> -    output_domain(reverse["OF1.0"], "ofperr_of10", "OpenFlow 1.0", 0x01)
> -    output_domain(reverse["OF1.1"], "ofperr_of11", "OpenFlow 1.1", 0x02)
> -    output_domain(reverse["OF1.2"], "ofperr_of12", "OpenFlow 1.2", 0x03)
> -    output_domain(reverse["OF1.3"], "ofperr_of13", "OpenFlow 1.3", 0x04)
> +    output_domain(reverse["1.0"], "ofperr_of10", "OpenFlow 1.0", 0x01)
> +    output_domain(reverse["1.1"], "ofperr_of11", "OpenFlow 1.1", 0x02)
> +    output_domain(reverse["1.2"], "ofperr_of12", "OpenFlow 1.2", 0x03)
> +    output_domain(reverse["1.3"], "ofperr_of13", "OpenFlow 1.3", 0x04)
>
>  if __name__ == '__main__':
>      if '--help' in sys.argv:
> diff --git a/include/openflow/openflow-1.2.h b/include/openflow/openflow-1.2.h
> index 5546313..2977047 100644
> --- a/include/openflow/openflow-1.2.h
> +++ b/include/openflow/openflow-1.2.h
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2008, 2011, 2012 The Board of Trustees of The Leland Stanford
> +/* Copyright (c) 2008, 2011, 2012, 2013 The Board of Trustees of The Leland Stanford
>   * Junior University
>   *
>   * We are making the OpenFlow specification and associated documentation
> @@ -55,6 +55,9 @@
>
>  #include "openflow/openflow-1.1.h"
>
> +/* Error type for experimenter error messages. */
> +#define OFPET12_EXPERIMENTER 0xffff
> +
>  /*
>   * OXM Class IDs.
>   * The high order bit differentiate reserved classes from member classes.
> diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
> index e2449b3..1cfa62d 100644
> --- a/lib/ofp-errors.c
> +++ b/lib/ofp-errors.c
> @@ -11,7 +11,8 @@
>
>  VLOG_DEFINE_THIS_MODULE(ofp_errors);
>
> -struct pair {
> +struct triplet {
> +    uint32_t vendor;
>      int type, code;
>  };
>
> @@ -57,10 +58,11 @@ ofperr_is_valid(enum ofperr error)
>   * 'version', or 0 if either no such OFPERR_* value exists or 'version' is
>   * unknown. */
>  static enum ofperr
> -ofperr_decode(enum ofp_version version, uint16_t type, uint16_t code)
> +ofperr_decode(enum ofp_version version,
> +              uint32_t vendor, uint16_t type, uint16_t code)
>  {
>      const struct ofperr_domain *domain = ofperr_domain_from_version(version);
> -    return domain ? domain->decode(type, code) : 0;
> +    return domain ? domain->decode(vendor, type, code) : 0;
>  }
>
>  /* Returns the name of 'error', e.g. "OFPBRC_BAD_TYPE" if 'error' is
> @@ -105,8 +107,8 @@ ofperr_get_description(enum ofperr error)
>              : "<invalid>");
>  }
>
> -static const struct pair *
> -ofperr_get_pair__(enum ofperr error, const struct ofperr_domain *domain)
> +static const struct triplet *
> +ofperr_get_triplet__(enum ofperr error, const struct ofperr_domain *domain)
>  {
>      size_t ofs = error - OFPERR_OFS;
>
> @@ -120,8 +122,8 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
>  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>      const struct ofperr_domain *domain;
> +    const struct triplet *triplet;
>      struct ofp_error_msg *oem;
> -    const struct pair *pair;
>      struct ofpbuf *buf;
>
>      /* Get the error domain for 'ofp_version', or fall back to OF1.0. */
> @@ -144,15 +146,15 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
>          error = OFPERR_NXBRC_UNENCODABLE_ERROR;
>      }
>
> -    pair = ofperr_get_pair__(error, domain);
> -    if (pair->code < 0x100) {
> +    triplet = ofperr_get_triplet__(error, domain);
> +    if (!triplet->vendor) {
>          buf = ofpraw_alloc_xid(OFPRAW_OFPT_ERROR, domain->version, xid,
>                                 sizeof *oem + data_len);
>
>          oem = ofpbuf_put_uninit(buf, sizeof *oem);
> -        oem->type = htons(pair->type);
> -        oem->code = htons(pair->code);
> -    } else {
> +        oem->type = htons(triplet->type);
> +        oem->code = htons(triplet->code);
> +    } else if (ofp_version <= OFP11_VERSION) {
>          struct nx_vendor_error *nve;
>
>          buf = ofpraw_alloc_xid(OFPRAW_OFPT_ERROR, domain->version, xid,
> @@ -163,9 +165,19 @@ ofperr_encode_msg__(enum ofperr error, enum ofp_version ofp_version,
>          oem->code = htons(NXVC_VENDOR_ERROR);
>
>          nve = ofpbuf_put_uninit(buf, sizeof *nve);
> -        nve->vendor = htonl(NX_VENDOR_ID);
> -        nve->type = htons(pair->type);
> -        nve->code = htons(pair->code);
> +        nve->vendor = htonl(triplet->vendor);
> +        nve->type = htons(triplet->type);
> +        nve->code = htons(triplet->code);
> +    } else {
> +        ovs_be32 vendor = htonl(triplet->vendor);
> +
> +        buf = ofpraw_alloc_xid(OFPRAW_OFPT_ERROR, domain->version, xid,
> +                               sizeof *oem + sizeof(uint32_t) + data_len);
> +
> +        oem = ofpbuf_put_uninit(buf, sizeof *oem);
> +        oem->type = htons(OFPET12_EXPERIMENTER);
> +        oem->code = htons(triplet->type);
> +        ofpbuf_put(buf, &vendor, sizeof vendor);
>      }
>
>      ofpbuf_put(buf, data, data_len);
> @@ -206,6 +218,13 @@ ofperr_encode_hello(enum ofperr error, enum ofp_version ofp_version,
>      return ofperr_encode_msg__(error, ofp_version, htonl(0), s, strlen(s));
>  }
>
> +int
> +ofperr_get_vendor(enum ofperr error, enum ofp_version version)
> +{
> +    const struct ofperr_domain *domain = ofperr_domain_from_version(version);
> +    return domain ? ofperr_get_triplet__(error, domain)->vendor : -1;
> +}
> +
>  /* Returns the value that would go into an OFPT_ERROR message's 'type' for
>   * encoding 'error' in 'domain'.  Returns -1 if 'error' is not encodable in
>   * 'version' or 'version' is unknown.
> @@ -215,7 +234,7 @@ int
>  ofperr_get_type(enum ofperr error, enum ofp_version version)
>  {
>      const struct ofperr_domain *domain = ofperr_domain_from_version(version);
> -    return domain ? ofperr_get_pair__(error, domain)->type : -1;
> +    return domain ? ofperr_get_triplet__(error, domain)->type : -1;
>  }
>
>  /* Returns the value that would go into an OFPT_ERROR message's 'code' for
> @@ -229,7 +248,7 @@ int
>  ofperr_get_code(enum ofperr error, enum ofp_version version)
>  {
>      const struct ofperr_domain *domain = ofperr_domain_from_version(version);
> -    return domain ? ofperr_get_pair__(error, domain)->code : -1;
> +    return domain ? ofperr_get_triplet__(error, domain)->code : -1;
>  }
>
>  /* Tries to decode 'oh', which should be an OpenFlow OFPT_ERROR message.
> @@ -240,12 +259,11 @@ ofperr_get_code(enum ofperr error, enum ofp_version version)
>  enum ofperr
>  ofperr_decode_msg(const struct ofp_header *oh, struct ofpbuf *payload)
>  {
> -    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> -
>      const struct ofp_error_msg *oem;
>      enum ofpraw raw;
>      uint16_t type, code;
>      enum ofperr error;
> +    uint32_t vendor;
>      struct ofpbuf b;
>
>      if (payload) {
> @@ -261,6 +279,7 @@ ofperr_decode_msg(const struct ofp_header *oh, struct ofpbuf *payload)
>      oem = ofpbuf_pull(&b, sizeof *oem);
>
>      /* Get the error type and code. */
> +    vendor = 0;
>      type = ntohs(oem->type);
>      code = ntohs(oem->code);
>      if (type == NXET_VENDOR && code == NXVC_VENDOR_ERROR) {
> @@ -269,17 +288,22 @@ ofperr_decode_msg(const struct ofp_header *oh, struct ofpbuf *payload)
>              return 0;
>          }
>
> -        if (nve->vendor != htonl(NX_VENDOR_ID)) {
> -            VLOG_WARN_RL(&rl, "error contains unknown vendor ID %#"PRIx32,
> -                         ntohl(nve->vendor));
> -            return 0;
> -        }
> +        vendor = ntohl(nve->vendor);
>          type = ntohs(nve->type);
>          code = ntohs(nve->code);
> +    } else if (type == OFPET12_EXPERIMENTER) {
> +        const ovs_be32 *vendorp = ofpbuf_try_pull(&b, sizeof *vendorp);
> +        if (!vendorp) {
> +            return 0;
> +        }
> +
> +        vendor = ntohl(*vendorp);
> +        type = code;
> +        code = 0;
>      }
>
>      /* Translate the error type and code into an ofperr. */
> -    error = ofperr_decode(oh->version, type, code);
> +    error = ofperr_decode(oh->version, vendor, type, code);
>      if (error && payload) {
>          ofpbuf_use_const(payload, b.data, b.size);
>      }
> diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
> index 1f7ea69..7da6f6d 100644
> --- a/lib/ofp-errors.h
> +++ b/lib/ofp-errors.h
> @@ -59,9 +59,13 @@ struct ofpbuf;
>   *     square brackets is omitted; this can be used to explain rationale for
>   *     choice of error codes in the case where this is desirable. */
>  enum ofperr {
> +/* Experimenter codes. */
> +
> +    /* Experimenter NX (Nicira Networks) = 0x00002320. */
> +
>  /* Expected duplications. */
>
> -    /* Expected: 3,5 in OF1.1 means both OFPBIC_BAD_EXPERIMENTER and
> +    /* Expected: 0x0,3,5 in OF1.1 means both OFPBIC_BAD_EXPERIMENTER and
>       * OFPBIC_BAD_EXP_TYPE. */
>
>  /* ## ------------------ ## */
> @@ -115,10 +119,10 @@ enum ofperr {
>      /* OF1.2+(1,10).  Denied because controller is slave. */
>      OFPERR_OFPBRC_IS_SLAVE,
>
> -    /* NX1.0(1,514), NX1.1(1,514), OF1.2+(1,11).  Invalid port.
> -     * [ A non-standard error (1,514), formerly
> -     *   OFPERR_NXBRC_BAD_IN_PORT is used for OpenFlow 1.0 and 1.1 as there
> -     *   seems to be no appropriate error code defined the specifications. ] */
> +    /* NX1.0-1.1(1,514), OF1.2+(1,11).  Invalid port.  [ A non-standard error
> +     * (1,514), formerly OFPERR_NXBRC_BAD_IN_PORT is used for OpenFlow 1.0 and
> +     * 1.1 as there seems to be no appropriate error code defined the
> +     * specifications. ] */
>      OFPERR_OFPBRC_BAD_PORT,
>
>      /* OF1.2+(1,12).  Invalid packet in packet-out. */
> @@ -127,41 +131,43 @@ enum ofperr {
>      /* OF1.3+(1,13).  Multipart request overflowed the assigned buffer. */
>      OFPERR_OFPBRC_MULTIPART_BUFFER_OVERFLOW,
>
> -    /* NX1.0+(1,256).  Invalid NXM flow match. */
> +    /* NX1.0-1.1(1,256), NX1.2+(2).  Invalid NXM flow match. */
>      OFPERR_NXBRC_NXM_INVALID,
>
> -    /* NX1.0+(1,257).  The nxm_type, or nxm_type taken in combination with
> -     * nxm_hasmask or nxm_length or both, is invalid or not implemented. */
> +    /* NX1.0-1.1(1,257), NX1.2+(3).  The nxm_type, or nxm_type taken in
> +     * combination with nxm_hasmask or nxm_length or both, is invalid or not
> +     * implemented. */
>      OFPERR_NXBRC_NXM_BAD_TYPE,
>
> -    /* NX1.0+(1,515).  Must-be-zero field had nonzero value. */
> +    /* NX1.0-1.1(1,515), NX1.2+(4).  Must-be-zero field had nonzero value. */
>      OFPERR_NXBRC_MUST_BE_ZERO,
>
> -    /* NX1.0+(1,516).  The reason in an ofp_port_status message is not
> -     * valid. */
> +    /* NX1.0-1.1(1,516), NX1.2+(5).  The reason in an ofp_port_status message
> +     * is not valid. */
>      OFPERR_NXBRC_BAD_REASON,
>
> -    /* NX1.0+(1,517).  The 'id' in an NXST_FLOW_MONITOR request is the same as
> -     * an existing monitor id (or two monitors in the same NXST_FLOW_MONITOR
> -     * request have the same 'id').  */
> +    /* NX1.0-1.1(1,517), NX1.2+(6).  The 'id' in an NXST_FLOW_MONITOR request
> +     * is the same as an existing monitor id (or two monitors in the same
> +     * NXST_FLOW_MONITOR request have the same 'id').  */
>      OFPERR_NXBRC_FM_DUPLICATE_ID,
>
> -    /* NX1.0+(1,518).  The 'flags' in an NXST_FLOW_MONITOR request either does
> -     * not specify at least one of the NXFMF_ADD, NXFMF_DELETE, or NXFMF_MODIFY
> -     * flags, or specifies a flag bit that is not defined. */
> +    /* NX1.0-1.1(1,518), NX1.2+(7).  The 'flags' in an NXST_FLOW_MONITOR
> +     * request either does not specify at least one of the NXFMF_ADD,
> +     * NXFMF_DELETE, or NXFMF_MODIFY flags, or specifies a flag bit that is not
> +     * defined. */
>      OFPERR_NXBRC_FM_BAD_FLAGS,
>
> -    /* NX1.0+(1,519).  The 'id' in an NXT_FLOW_MONITOR_CANCEL request is not
> -     * the id of any existing monitor. */
> +    /* NX1.0-1.1(1,519), NX1.2+(8).  The 'id' in an NXT_FLOW_MONITOR_CANCEL
> +     * request is not the id of any existing monitor. */
>      OFPERR_NXBRC_FM_BAD_ID,
>
> -    /* NX1.0+(1,520).  The 'event' in an NXST_FLOW_MONITOR reply does not
> -     * specify one of the NXFME_ABBREV, NXFME_ADD, NXFME_DELETE, or
> +    /* NX1.0-1.1(1,520), NX1.2+(9).  The 'event' in an NXST_FLOW_MONITOR reply
> +     * does not specify one of the NXFME_ABBREV, NXFME_ADD, NXFME_DELETE, or
>       * NXFME_MODIFY. */
>      OFPERR_NXBRC_FM_BAD_EVENT,
>
> -    /* NX1.0+(1,521).  The error that occurred cannot be represented in this
> -     * OpenFlow version. */
> +    /* NX1.0-1.1(1,521), NX1.2+(10).  The error that occurred cannot be
> +     * represented in this OpenFlow version. */
>      OFPERR_NXBRC_UNENCODABLE_ERROR,
>
>  /* ## ---------------- ## */
> @@ -217,7 +223,8 @@ enum ofperr {
>      /* OF1.2+(2,15).  Bad argument in SET_FIELD action. */
>      OFPERR_OFPBAC_ARGUMENT,
>
> -    /* NX1.0+(2,256).  Must-be-zero action argument had nonzero value. */
> +    /* NX1.0-1.1(2,256), NX1.2+(11).  Must-be-zero action argument had nonzero
> +     * value. */
>      OFPERR_NXBAC_MUST_BE_ZERO,
>
>  /* ## --------------------- ## */
> @@ -282,15 +289,14 @@ enum ofperr {
>       * field. */
>      OFPERR_OFPBMC_BAD_VALUE,
>
> -    /* NX1.0(1,259), NX1.1(1,259), OF1.2+(4,8).  Unsupported mask specified in
> -     * the match, field is not dl-address or nw-address. */
> +    /* NX1.0-1.1(1,259), OF1.2+(4,8).  Unsupported mask specified in the match,
> +     * field is not dl-address or nw-address. */
>      OFPERR_OFPBMC_BAD_MASK,
>
> -    /* NX1.0(1,260), NX1.1(1,260), OF1.2+(4,9).  A prerequisite was not met. */
> +    /* NX1.0-1.1(1,260), OF1.2+(4,9).  A prerequisite was not met. */
>      OFPERR_OFPBMC_BAD_PREREQ,
>
> -    /* NX1.0(1,261), NX1.1(1,261), OF1.2+(4,10).  A field type was
> -     * duplicated. */
> +    /* NX1.0-1.1(1,261), OF1.2+(4,10).  A field type was duplicated. */
>      OFPERR_OFPBMC_DUP_FIELD,
>
>      /* OF1.2+(4,11).  Permissions error. */
> @@ -333,12 +339,12 @@ enum ofperr {
>       * specified. */
>      OFPERR_OFPFMFC_UNSUPPORTED,
>
> -    /* NX1.0(3,256), NX1.1(5,256).  Generic hardware error. */
> +    /* NX1.0-1.1(5,256), NX1.2+(12).  Generic hardware error. */
>      OFPERR_NXFMFC_HARDWARE,
>
> -    /* NX1.0(3,257), NX1.1(5,257).  A nonexistent table ID was specified in the
> -     * "command" field of struct ofp_flow_mod, when the nxt_flow_mod_table_id
> -     * extension is enabled. */
> +    /* NX1.0-1.1(5,257), NX1.2+(13).  A nonexistent table ID was specified in
> +     * the "command" field of struct ofp_flow_mod, when the
> +     * nxt_flow_mod_table_id extension is enabled. */
>      OFPERR_NXFMFC_BAD_TABLE_ID,
>
>  /* ## ---------------------- ## */
> @@ -465,7 +471,7 @@ enum ofperr {
>      /* OF1.2+(11,1).  Controller role change unsupported. */
>      OFPERR_OFPRRFC_UNSUP,
>
> -    /* NX1.0(1,513), NX1.1(1,513), OF1.2+(11,2).  Invalid role. */
> +    /* NX1.0-1.1(1,513), OF1.2+(11,2).  Invalid role. */
>      OFPERR_OFPRRFC_BAD_ROLE,
>
>  /* ## ---------------------- ## */
> @@ -549,6 +555,7 @@ enum ofperr ofperr_decode_msg(const struct ofp_header *,
>  struct ofpbuf *ofperr_encode_reply(enum ofperr, const struct ofp_header *);
>  struct ofpbuf *ofperr_encode_hello(enum ofperr, enum ofp_version ofp_version,
>                                     const char *);
> +int ofperr_get_vendor(enum ofperr, enum ofp_version);
>  int ofperr_get_type(enum ofperr, enum ofp_version);
>  int ofperr_get_code(enum ofperr, enum ofp_version);
>
> diff --git a/tests/ofp-errors.at b/tests/ofp-errors.at
> index e99aca9..f39a213 100644
> --- a/tests/ofp-errors.at
> +++ b/tests/ofp-errors.at
> @@ -87,16 +87,14 @@ dnl Thus, for OF1.1 we translate both of the latter error codes into 3,5.
>  AT_SETUP([encoding OFPBIC_* experimenter errors])
>  AT_KEYWORDS([ofp-print ofp-errors])
>  AT_CHECK([ovs-ofctl print-error OFPBIC_BAD_EXPERIMENTER], [0], [dnl
> -OpenFlow 1.0: -1,-1
> -OpenFlow 1.1: 3,5
> -OpenFlow 1.2: 3,5
> -OpenFlow 1.3: 3,5
> +OpenFlow 1.1: vendor 0, type 3, code 5
> +OpenFlow 1.2: vendor 0, type 3, code 5
> +OpenFlow 1.3: vendor 0, type 3, code 5
>  ])
>  AT_CHECK([ovs-ofctl print-error OFPBIC_BAD_EXP_TYPE], [0], [dnl
> -OpenFlow 1.0: -1,-1
> -OpenFlow 1.1: 3,5
> -OpenFlow 1.2: 3,6
> -OpenFlow 1.3: 3,6
> +OpenFlow 1.1: vendor 0, type 3, code 5
> +OpenFlow 1.2: vendor 0, type 3, code 6
> +OpenFlow 1.3: vendor 0, type 3, code 6
>  ])
>  AT_CLEANUP
>
> @@ -127,14 +125,74 @@ AT_CHECK([ovs-ofctl ofp-print '0201001455555555 00030005 0102000811111111'], [0]
>  OFPT_ERROR (OF1.1) (xid=0x55555555): OFPBIC_BAD_EXPERIMENTER
>  OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
>  ])
> -AT_KEYWORDS([ofp-print ofp-errors])
>  AT_CHECK([ovs-ofctl ofp-print '0301001455555555 00030005 0102000811111111'], [0], [dnl
>  OFPT_ERROR (OF1.2) (xid=0x55555555): OFPBIC_BAD_EXPERIMENTER
>  OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
>  ])
> -AT_KEYWORDS([ofp-print ofp-errors])
>  AT_CHECK([ovs-ofctl ofp-print '0301001455555555 00030006 0102000811111111'], [0], [dnl
>  OFPT_ERROR (OF1.2) (xid=0x55555555): OFPBIC_BAD_EXP_TYPE
>  OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
>  ])
>  AT_CLEANUP
> +
> +AT_SETUP([decoding experimenter errors])
> +AT_KEYWORDS([ofp-print ofp-errors])
> +AT_CHECK([ovs-ofctl ofp-print '0101001c55555555 b0c20000 0000232000010203 0102000811111111'], [0], [dnl
> +OFPT_ERROR (xid=0x55555555): NXBRC_MUST_BE_ZERO
> +OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0201001c55555555 b0c20000 0000232000010203 0102000811111111'], [0], [dnl
> +OFPT_ERROR (OF1.1) (xid=0x55555555): NXBRC_MUST_BE_ZERO
> +OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0301001855555555 ffff0004 00002320 0102000811111111'], [0], [dnl
> +OFPT_ERROR (OF1.2) (xid=0x55555555): NXBRC_MUST_BE_ZERO
> +OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0101001c55555555 b0c20000 00002320ffff0001 0102000811111111'], [0], [dnl
> +OFPT_ERROR (xid=0x55555555): NX_DUP_INSTRUCTION
> +OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0201001c55555555 b0c20000 00002320ffff0001 0102000811111111'], [0], [dnl
> +OFPT_ERROR (OF1.1) (xid=0x55555555): NX_DUP_INSTRUCTION
> +OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> +])
> +AT_CHECK([ovs-ofctl ofp-print '0301001855555555 ffff0001 00002320 0102000811111111'], [0], [dnl
> +OFPT_ERROR (OF1.2) (xid=0x55555555): NX_DUP_INSTRUCTION
> +OFPT_ECHO_REQUEST (xid=0x11111111): 0 bytes of payload
> +])
> +AT_CLEANUP
> +
> +AT_SETUP([encoding experimenter errors])
> +AT_KEYWORDS([ofp-print ofp-errors])
> +AT_CHECK(
> +  [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0100000812345678], [0], [dnl
> +00000000  01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> +00000010  00 01 02 03 01 00 00 08-12 34 56 78 @&t@
> +])
> +AT_CHECK(
> +  [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0200000812345678], [0], [dnl
> +00000000  02 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> +00000010  00 01 02 03 02 00 00 08-12 34 56 78 @&t@
> +])
> +AT_CHECK(
> +  [ovs-ofctl encode-error-reply NXBRC_MUST_BE_ZERO 0300000812345678], [0], [dnl
> +00000000  03 01 00 18 12 34 56 78-ff ff 00 04 00 00 23 20 @&t@
> +00000010  03 00 00 08 12 34 56 78-
> +])
> +AT_CHECK(
> +  [ovs-ofctl encode-error-reply NX_DUP_INSTRUCTION 0100000812345678], [0],
> +[00000000  01 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> +00000010  ff ff 00 01 01 00 00 08-12 34 56 78 @&t@
> +])
> +AT_CHECK(
> +  [ovs-ofctl encode-error-reply NX_DUP_INSTRUCTION 0200000812345678], [0],
> +[00000000  02 01 00 1c 12 34 56 78-b0 c2 00 00 00 00 23 20 @&t@
> +00000010  ff ff 00 01 02 00 00 08-12 34 56 78 @&t@
> +])
> +AT_CHECK(
> +  [ovs-ofctl encode-error-reply NX_DUP_INSTRUCTION 0300000812345678], [0],
> +[00000000  03 01 00 18 12 34 56 78-ff ff 00 01 00 00 23 20 @&t@
> +00000010  03 00 00 08 12 34 56 78-
> +])
> +AT_CLEANUP
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index ec775c7..cbdfba6 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -2747,13 +2747,16 @@ ofctl_print_error(int argc OVS_UNUSED, char *argv[])
>
>      for (version = 0; version <= UINT8_MAX; version++) {
>          const char *name = ofperr_domain_get_name(version);
> -        if (!name) {
> -            continue;
> +        if (name) {
> +            int vendor = ofperr_get_vendor(error, version);
> +            int type = ofperr_get_type(error, version);
> +            int code = ofperr_get_code(error, version);
> +
> +            if (vendor != -1 || type != -1 || code != -1) {
> +                printf("%s: vendor %#x, type %d, code %d\n",
> +                       name, vendor, type, code);
> +            }
>          }
> -        printf("%s: %d,%d\n",
> -               ofperr_domain_get_name(version),
> -               ofperr_get_type(error, version),
> -               ofperr_get_code(error, version));
>      }
>  }
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list