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

Alex Wang alexw at nicira.com
Fri Jun 21 05:37:08 UTC 2013


Hey Ben,

The code makes sense, have few question inline,


On Wed, Jun 12, 2013 at 11:35 AM, 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.
>
> This commit also makes it easy to add error codes for new experimenter IDs
> by adding new *_VENDOR_ID definitions to openflow-common.h.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  OPENFLOW-1.1+                      |    3 -
>  build-aux/extract-ofp-errors       |  326
> ++++++++++++++++++++++--------------
>  include/openflow/nicira-ext.h      |    1 -
>  include/openflow/openflow-1.2.h    |    5 +-
>  include/openflow/openflow-common.h |   25 +++-
>  lib/automake.mk                    |    9 +-
>  lib/ofp-errors.c                   |   72 ++++++---
>  lib/ofp-errors.h                   |   89 ++++++----
>  tests/ofp-actions.at               |    4 +-
>  tests/ofp-errors.at                |   51 +++++-
>  utilities/ovs-ofctl.c              |   15 +-
>  11 files changed, 384 insertions(+), 216 deletions(-)
>
> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
> index c4bcc34..fb39873 100644
> --- a/OPENFLOW-1.1+
> +++ b/OPENFLOW-1.1+
> @@ -92,9 +92,6 @@ OpenFlow 1.2 support requires OpenFlow 1.1 as a
> prerequisite, plus the
>  following additional work.  (This is based on the change log at the
>  end of the OF1.2 spec.  I didn't compare the specs carefully yet.)
>
> -    * Use new OpenFlow extensible error infrastructure, on OF1.2+
> -      only, instead of the OVS-specific extension used until now.
> -
>      * OFPT_FLOW_MOD:
>
>          * MODIFY and MODIFY_STRICT commands now never insert new flows
> diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
> index 32d0913..5378dac 100755
> --- a/build-aux/extract-ofp-errors
> +++ b/build-aux/extract-ofp-errors
> @@ -6,6 +6,13 @@ import re
>
>  macros = {}
>
> +# Map from OpenFlow version number to version ID used in ofp_header.
> +version_map = {"1.0": 0x01,
> +               "1.1": 0x02,
> +               "1.2": 0x03,
> +               "1.3": 0x04}
> +version_reverse_map = dict((v, k) for (k, v) in version_map.iteritems())
> +
>  token = None
>  line = ""
>  idRe = "[a-zA-Z_][a-zA-Z_0-9]*"
> @@ -13,12 +20,24 @@ tokenRe = "#?" + idRe + "|[0-9]+|."
>  inComment = False
>  inDirective = False
>
> -def getLine():
> +def open_file(fn):
> +    global fileName
> +    global inputFile
> +    global lineNumber
> +    fileName = fn
> +    inputFile = open(fileName)
> +    lineNumber = 0
> +
> +def tryGetLine():
> +    global inputFile
>      global line
>      global lineNumber
>      line = inputFile.readline()
>      lineNumber += 1
> -    if line == "":
> +    return line != ""
> +
> +def getLine():
> +    if not tryGetLine():
>          fatal("unexpected end of input")
>
>  def getToken():
> @@ -133,146 +152,190 @@ def usage():
>      argv0 = os.path.basename(sys.argv[0])
>      print ('''\
>  %(argv0)s, for extracting OpenFlow error codes from header files
> -usage: %(argv0)s FILE [FILE...]
> +usage: %(argv0)s ERROR_HEADER VENDOR_HEADER
>
> -This program reads the header files specified on the command line and
> -outputs a C source file for translating OpenFlow error codes into
> -strings, for use as lib/ofp-errors.c in the Open vSwitch source tree.
> +This program reads VENDOR_HEADER to obtain OpenFlow vendor (aka
> +experimenter IDs), then ERROR_HEADER to obtain OpenFlow error number.
> +It outputs a C source file for translating OpenFlow error codes into
> +strings.
>
> -This program is specialized for reading lib/ofp-errors.h.  It will not
> -work on arbitrary header files without extensions.\
> +ERROR_HEADER should point to lib/ofp-errors.h.
> +VENDOR_HEADER should point to include/openflow/openflow-common.h.
> +The output is suitable for use as lib/ofp-errors.inc.\
>  ''' % {"argv0": argv0})
>      sys.exit(0)
>
> -def extract_ofp_errors(filenames):
> +def extract_vendor_ids(fn):
> +    global vendor_map
> +    vendor_map = {}
> +    vendor_loc = {}
> +
> +    open_file(fn)
> +    while tryGetLine():
> +        m =
> re.match(r'#define\s+([A-Z0-9_]+)_VENDOR_ID\s+(0x[0-9a-fA-F]+|[0-9]+)',
> line)
> +        if not m:
> +            continue
> +
> +        name = m.group(1)
> +        id_ = int(m.group(2), 0)
> +
> +        if name in vendor_map:
> +            error("%s: duplicate definition of vendor" % name)
> +            sys.stderr.write("%s: Here is the location of the previous "
> +                             "definition.\n" % vendor_loc[name])
> +            sys.exit(1)
> +
> +        vendor_map[name] = id_
> +        vendor_loc[name] = "%s:%d" % (fileName, lineNumber)
> +
> +    if not vendor_map:
> +        fatal("%s: no vendor definitions found" % fn)
> +
> +    inputFile.close()
> +
> +    vendor_reverse_map = {}
> +    for name, id_ in vendor_map.items():
> +        if id_ in vendor_reverse_map:
> +            fatal("%s: duplicate vendor id for vendors %s and %s"
> +                  % (id_, vendor_reverse_map[id_], name))
> +        vendor_reverse_map[id_] = name
> +
> +def extract_ofp_errors(fn):
>      error_types = {}
>
>      comments = []
>      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 version_map.values():
>          domain[domain_name] = {}
>          reverse[domain_name] = {}
>
>      n_errors = 0
>      expected_errors = {}
>
> -    global fileName
> -    for fileName in filenames:
> -        global inputFile
> -        global lineNumber
> -        inputFile = open(fileName)
> -        lineNumber = 0
> +    open_file(fn)
>
> -        while True:
> -            getLine()
> -            if re.match('enum ofperr', line):
> -                break
> +    while True:
> +        getLine()
> +        if re.match('enum ofperr', line):
> +            break
> +
> +    while True:
> +        getLine()
> +        if line.startswith('/*') or not line or line.isspace():
> +            continue
> +        elif re.match('}', line):
> +            break
> +
> +        if not line.lstrip().startswith('/*'):
> +            fatal("unexpected syntax between errors")
>
> -        while True:
> +        comment = line.lstrip()[2:].strip()
> +        while not comment.endswith('*/'):
>              getLine()
>              if line.startswith('/*') or not line or line.isspace():
> -                continue
> -            elif re.match('}', line):
> -                break
> -
> -            if not line.lstrip().startswith('/*'):
> -                fatal("unexpected syntax between errors")
> -
> -            comment = line.lstrip()[2:].strip()
> -            while not comment.endswith('*/'):
> -                getLine()
> -                if line.startswith('/*') or not line or line.isspace():
> -                    fatal("unexpected syntax within error")
> -                comment += ' %s' % line.lstrip('* \t').rstrip(' \t\r\n')
> -            comment = comment[:-2].rstrip()
> -
> -            m = re.match('Expected: (.*)\.$', comment)
> -            if m:
> -                expected_errors[m.group(1)] = (fileName, lineNumber)
> -                continue
> +                fatal("unexpected syntax within error")
> +            comment += ' %s' % line.lstrip('* \t').rstrip(' \t\r\n')
> +        comment = comment[:-2].rstrip()
>
> -            m = re.match('((?:.(?!\.  ))+.)\.  (.*)$', comment)
> -            if not m:
> -                fatal("unexpected syntax between errors")
> +        m = re.match('Expected: (.*)\.$', comment)
> +        if m:
> +            expected_errors[m.group(1)] = (fileName, lineNumber)
> +            continue
>
> -            dsts, comment = m.groups()
> +        m = re.match('((?:.(?!\.  ))+.)\.  (.*)$', comment)
> +        if not m:
> +            fatal("unexpected syntax between errors")
>
> -            getLine()
> -            m =
> re.match('\s+(?:OFPERR_((?:OFP|NX)[A-Z0-9_]+))(\s*=\s*OFPERR_OFS)?,',
> -                         line)
> -            if not m:
> -                fatal("syntax error expecting enum value")
> +        dsts, comment = m.groups()
>
> -            enum = m.group(1)
> +        getLine()
> +        m = re.match('\s+(?:OFPERR_([A-Z0-9_]+))(\s*=\s*OFPERR_OFS)?,',
> +                     line)
> +        if not m:
> +            fatal("syntax error expecting enum value")
>
> -            comments.append(re.sub('\[[^]]*\]', '', comment))
> -            names.append(enum)
> +        enum = m.group(1)
>
> -            for dst in dsts.split(', '):
> -                m = re.match(r'([A-Z0-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))
> -                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)
> -                        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],
> -                                                dst))
> +        comments.append(re.sub('\[[^]]*\]', '', comment))
> +        names.append(enum)
> +
> +        for dst in dsts.split(', '):
> +            m =
> re.match(r'([A-Z]+)([0-9.]+)(\+|-[0-9.]+)?\((\d+)(?:,(\d+))?\)$', dst)
> +            if not m:
> +                fatal("%r: syntax error in destination" % dst)
> +            vendor_name = m.group(1)
> +            version1_name = m.group(2)
> +            version2_name = m.group(3)
> +            type_ = int(m.group(4))
> +            if m.group(5):
> +                code = int(m.group(5))
> +            else:
> +                code = None
> +
> +            if vendor_name not in vendor_map:
> +                fatal("%s: unknown vendor" % vendor_name)
> +            vendor = vendor_map[vendor_name]
> +
> +            if version1_name not in version_map:
> +                fatal("%s: unknown OpenFlow version" % version1_name)
> +            v1 = version_map[version1_name]
> +
> +            if version2_name is None:
> +                v2 = v1
> +            elif version2_name == "+":
> +                v2 = max(version_map.values())
> +            elif version2_name[1:] not in version_map:
> +                fatal("%s: unknown OpenFlow version" % version2_name[1:])
> +            else:
> +                v2 = version_map[version2_name[1:]]
> +
> +            if v2 < v1:
> +                fatal("%s%s: %s precedes %s"
> +                      % (version1_name, version2_name,
> +                         version2_name, version1_name))
> +
> +            if vendor == vendor_map['NX']:
> +                if v1 >= version_map['1.2'] or v2 >= version_map['1.2']:
> +                    if code is not None:
> +                        fatal("%s: NX1.2+ domains do not have codes" %
> dst)
> +                    code = 0
> +            elif vendor != vendor_map['OF']:
> +                if code is not None:
> +                    fatal("%s: %s domains do not have codes" %
> vendor_name)
> +
> +            for version in range(v1, v2 + 1):
> +                domain[version].setdefault(vendor, {})
> +                domain[version][vendor].setdefault(type_, {})
> +                if code in domain[version][vendor][type_]:
> +                    msg = "%#x,%d,%d in OF%s means both %s and %s" % (
> +                        vendor, type_, code, version_reverse_map[version],
> +                        domain[version][vendor][type_][code][0], enum)
> +                    if msg in expected_errors:
> +                        del expected_errors[msg]
>                      else:
> -                        domain[target][type_][code] = (enum, fileName,
> -                                                       lineNumber)
> +                        error("%s: %s." % (dst, msg))
> +                        sys.stderr.write("%s:%d: %s: Here is the location
> "
> +                                         "of the previous definition.\n"
> +                                         %
> (domain[version][vendor][type_][code][1],
> +
>  domain[version][vendor][type_][code][2],
> +                                            dst))
> +                else:
> +                    domain[version][vendor][type_][code] = (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_map[version],
> +                           reverse[version][enum][0],
> +                           reverse[version][enum][1],
> +                           reverse[version][enum][2],
> +                           vendor, type_, code))
> +                reverse[version][enum] = (vendor, type_, code)
>



Want to ask here, I'm not sure whether "if enum in reverse[version]" will
ever be true. Because, if it is true, that means there are two exactly same
enum strings, which is impossible. The compiler will complain if two
entries in the enumeration have same name.




> -        inputFile.close()
> +    inputFile.close()
>
>      for fn, ln in expected_errors.values():
>          sys.stderr.write("%s:%d: expected duplicate not used.\n" % (fn,
> ln))
> @@ -289,8 +352,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 +371,25 @@ 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,27 +405,28 @@ 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)
> +    for version_name, id_ in version_map.items():
> +        var = 'ofperr_of' + re.sub('[^A-Za-z0-9_]', '', version_name)
> +        description = "OpenFlow %s" % version_name
> +        output_domain(reverse[id_], var, description, id_)
>
>  if __name__ == '__main__':
>      if '--help' in sys.argv:
>          usage()
> -    elif len(sys.argv) < 2:
> -        sys.stderr.write("at least one non-option argument required; "
> +    elif len(sys.argv) != 3:
> +        sys.stderr.write("exactly two non-options arguments required; "
>                           "use --help for help\n")
>          sys.exit(1)
>      else:
> -        extract_ofp_errors(sys.argv[1:])
> +        extract_vendor_ids(sys.argv[2])
> +        extract_ofp_errors(sys.argv[1])
> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> index 5ca343e..6319f4e 100644
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -24,7 +24,6 @@
>   * standardized, so they are not included in openflow.h.  Some of them
> may be
>   * suitable for standardization; others we never expect to standardize. */
>
> -#define NX_VENDOR_ID 0x00002320
>
>  /* Nicira vendor-specific error messages extension.
>   *
> 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/include/openflow/openflow-common.h
> b/include/openflow/openflow-common.h
> index 3cc22c9..9759f82 100644
> --- a/include/openflow/openflow-common.h
> +++ b/include/openflow/openflow-common.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
> @@ -78,6 +78,29 @@ enum ofp_version {
>      OFP13_VERSION = 0x04
>  };
>
> +/* Vendor (aka experimenter) IDs.
> + *
> + * These are used in various places in OpenFlow to identify an extension
> + * defined by some vendor, as opposed to a standardized part of the core
> + * OpenFlow protocol.
> + *
> + * Vendor IDs whose top 8 bits are 0 hold an Ethernet OUI in their low 24
> bits.
> + * The Open Networking Foundation assigns vendor IDs whose top 8 bits are
> + * nonzero.
> + *
> + * A few vendor IDs are special:
> + *
> + *    - OF_VENDOR_ID is not a real vendor ID and does not appear in the
> + *      OpenFlow protocol itself.  It can occasionally be useful within
> Open
> + *      vSwitch to identify a standardized part of OpenFlow.
> + *
> + *    - ONF_VENDOR_ID is being used within the ONF "extensibility" working
> + *      group to identify extensions being proposed for standardization.
> + */
> +#define OF_VENDOR_ID    0
> +#define NX_VENDOR_ID    0x00002320 /* Nicira. */
> +#define ONF_VENDOR_ID   0x4f4e4600 /* Open Networking Foundation. */
> +
>  #define OFP_MAX_TABLE_NAME_LEN 32
>  #define OFP_MAX_PORT_NAME_LEN  16
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index bcaa1f8..f22dab4 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -1,4 +1,4 @@
> -# Copyright (C) 2009, 2010, 2011, 2012 Nicira, Inc.
> +# Copyright (C) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>  #
>  # Copying and distribution of this file, with or without modification,
>  # are permitted in any medium without royalty provided the copyright
> @@ -337,9 +337,12 @@ lib/dirs.c: lib/dirs.c.in Makefile
>         mv lib/dirs.c.tmp lib/dirs.c
>
>  $(srcdir)/lib/ofp-errors.inc: \
> -       lib/ofp-errors.h $(srcdir)/build-aux/extract-ofp-errors
> +       lib/ofp-errors.h include/openflow/openflow-common.h \
> +       $(srcdir)/build-aux/extract-ofp-errors
>         $(run_python) $(srcdir)/build-aux/extract-ofp-errors \
> -               $(srcdir)/lib/ofp-errors.h > $@.tmp && mv $@.tmp $@
> +               $(srcdir)/lib/ofp-errors.h \
> +               $(srcdir)/include/openflow/openflow-common.h > $@.tmp
> +       mv $@.tmp $@
>  $(srcdir)/lib/ofp-errors.c: $(srcdir)/lib/ofp-errors.inc
>  EXTRA_DIST += build-aux/extract-ofp-errors lib/ofp-errors.inc
>
> diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
> index a141f3f..5285645 100644
> --- a/lib/ofp-errors.c
> +++ b/lib/ofp-errors.c
> @@ -27,7 +27,8 @@
>
>  VLOG_DEFINE_THIS_MODULE(ofp_errors);
>
> -struct pair {
> +struct triplet {
> +    uint32_t vendor;
>      int type, code;
>  };
>
> @@ -73,10 +74,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
> @@ -121,8 +123,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;
>
> @@ -136,8 +138,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. */
> @@ -160,15 +162,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,
> @@ -179,9 +181,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);
> @@ -222,6 +234,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.
> @@ -231,7 +250,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
> @@ -245,7 +264,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.
> @@ -256,12 +275,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) {
> @@ -277,6 +295,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) {
> @@ -285,17 +304,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..d144793 100644
> --- a/lib/ofp-errors.h
> +++ b/lib/ofp-errors.h
> @@ -51,8 +51,22 @@ struct ofpbuf;
>   * time and used to determine the mapping between "enum ofperr" constants
> and
>   * error type/code values used in the OpenFlow protocol:
>   *
> - *   - The first part of each comment specifies OpenFlow type/code for
> each
> - *     protocol that supports the error.
> + *   - The first part of each comment specifies the vendor, OpenFlow
> versions,
> + *     type, and sometimes a code for each protocol that supports the
> error:
> + *
> + *         # The vendor is OF for standard OpenFlow error codes.
>  Otherwise it
> + *           is one of the *_VENDOR_ID codes defined in openflow-common.h.
> + *
> + *         # The version can specify a specific OpenFlow version, a
> version
> + *           range delimited by "-", or an open-ended range with "+".
> + *
> + *         # Standard OpenFlow errors have both a type and a code.
>  Extension
> + *           errors generally have only a type, no code.  There is one
> + *           exception: Nicira extension (NX) errors for OpenFlow 1.0 and
> 1.1
> + *           have both a type and a code.  (This means that the version
> + *           specification for NX errors may not include version 1.0 or
> 1.1 (or
> + *           both) along with version 1.2 or later, because the
> requirements
> + *           for those versions are different.)
>   *
>   *   - Additional text is a human-readable description of the meaning of
> each
>   *     error, used to explain the error to the user.  Any text enclosed in
> @@ -61,7 +75,7 @@ struct ofpbuf;
>  enum ofperr {
>  /* 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 +129,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 +141,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 +233,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 +299,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 +349,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 +481,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,
>



For NX, "(1,513)", do the type and code here correspond to "struct
nx_vendor_error 's type and code"?

What is the rule of defining NX code here?

Also, seems to me that now, the legit vendors (with assigned vendor id) can
freely define their own error messages, right?



>  /* ## ---------------------- ## */
> @@ -549,6 +565,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-actions.at b/tests/ofp-actions.at
> index b455bb9..6cd3b42 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -339,7 +339,7 @@ dnl Check that an empty Apply-Actions instruction gets
> dropped.
>  0004 0008 00000000
>
>  dnl Duplicate instruction type:
> -# bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER
> +# bad OF1.1 instructions: NXBIC_DUP_INSTRUCTION
>  0004 0008 00000000 0004 0008 00000000
>
>  dnl Instructions not multiple of 8 in length.
> @@ -381,7 +381,7 @@ dnl Write-Metadata too long.
>  0002 0020 00000000 fedcba9876543210 ffffffffffffffff 0000000000000000
>
>  dnl Write-Metadata duplicated.
> -# bad OF1.1 instructions: OFPBAC_UNSUPPORTED_ORDER
> +# bad OF1.1 instructions: NXBIC_DUP_INSTRUCTION
>  0002 0018 00000000 fedcba9876543210 ff00ff00ff00ff00 0002 0018 00000000
> fedcba9876543210 ff00ff00ff00ff00
>


Want to ask what does NXBIC stand for? Also, I didn't find the definition
of this macro, even before applying patch 5/5. Want to know how and where
is it defined?



>  dnl Write-Metadata in wrong position (OpenFlow 1.1+ disregards the order
> diff --git a/tests/ofp-errors.at b/tests/ofp-errors.at
> index e99aca9..92f2a98 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,47 @@ 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_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_CLEANUP
>


I'm very curious about the en/decoding implementation here. So, I actually
followed the code
and decoded it myself. And I really want to ask an unrelated question.

I saw in the "ofperr_encode_msg__()" function in "lib/ofp-error.c", the
fields are converted to
network order, like below:

"
        oem = ofpbuf_put_uninit(buf, sizeof *oem);
        oem->type = htons(NXET_VENDOR);
"

In the "ovs_hex_dump()" function which prints the encoded header, it uses
"fprintf(stream, "%02hhx"
to print it and the value is again in host order (if %x is used, it will
print the network order).

So I want to ask what is the use of "hhx" here? and why can't we get rid of
htons and use %x in
ovs_hex_dump?



> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 48f0fbf..06acd15 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -2776,13 +2776,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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130620/f85e7ee8/attachment-0003.html>


More information about the dev mailing list