[ovs-dev] [PATCH 1/2] ofp-msgs: Add support for ONF extension messages.

Ben Pfaff blp at ovn.org
Wed Feb 24 22:45:46 UTC 2016


I applied these already, following your review.

On Wed, Feb 24, 2016 at 02:44:05PM -0800, Jarno Rajahalme wrote:
> Ping.
> 
> > On Feb 18, 2016, at 10:46 AM, Jarno Rajahalme <jarno at ovn.org> wrote:
> > 
> > I was able to implement the Extension 230 on top of these, so looking good :-)
> > 
> > Acked-by: Jarno Rajahalme <jarno at ovn.org>
> > 
> >> On Jan 27, 2016, at 3:50 PM, Ben Pfaff <blp at ovn.org> wrote:
> >> 
> >> ONF introduced a number of "standard extensions" that use its own
> >> vendor (experimenter) ID.  This commit adds support for such extensions to
> >> ofp-msgs.
> >> 
> >> These extensions were already half-supported, so there's barely any change
> >> to build-aux/extract-ofp-msgs.
> >> 
> >> This isn't fully tested, since nothing adds support for such a message yet.
> >> 
> >> Requested-by: Jarno Rajahalme <jarno at ovn.org>
> >> Signed-off-by: Ben Pfaff <blp at ovn.org>
> >> ---
> >> build-aux/extract-ofp-msgs         |  2 +-
> >> include/openflow/nicira-ext.h      | 16 --------
> >> include/openflow/openflow-1.1.h    | 11 ------
> >> include/openflow/openflow-common.h | 11 ------
> >> lib/ofp-msgs.c                     | 75 ++++++++++++++++++++++----------------
> >> lib/ofp-msgs.h                     | 17 ++++++---
> >> 6 files changed, 56 insertions(+), 76 deletions(-)
> >> 
> >> diff --git a/build-aux/extract-ofp-msgs b/build-aux/extract-ofp-msgs
> >> index b00039d..d3c4d4c 100755
> >> --- a/build-aux/extract-ofp-msgs
> >> +++ b/build-aux/extract-ofp-msgs
> >> @@ -161,7 +161,7 @@ def extract_ofp_msgs(output_file_name):
> >>                    hdrs = (version, OFPT10_STATS_REPLY, number, 0, 0)
> >>                else:
> >>                    hdrs = (version, OFPT11_STATS_REPLY, number, 0, 0)
> >> -            elif type_ == 'ONF':
> >> +            elif type_ == 'ONFT':
> >>                hdrs = (version, OFPT_VENDOR, 0, ONF_VENDOR_ID, number)
> >>            elif type_ == 'ONFST' and name.endswith('_REQUEST'):
> >>                if version == VERSION["1.0"]:
> >> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> >> index dad8707..fdee330 100644
> >> --- a/include/openflow/nicira-ext.h
> >> +++ b/include/openflow/nicira-ext.h
> >> @@ -68,14 +68,6 @@ struct nx_vendor_error {
> >> 
> >> /* Nicira vendor requests and replies. */
> >> 
> >> -/* Header for Nicira vendor requests and replies. */
> >> -struct nicira_header {
> >> -    struct ofp_header header;
> >> -    ovs_be32 vendor;            /* NX_VENDOR_ID. */
> >> -    ovs_be32 subtype;           /* See the NXT numbers in ofp-msgs.h. */
> >> -};
> >> -OFP_ASSERT(sizeof(struct nicira_header) == 16);
> >> -
> >> /* Header for Nicira vendor stats request and reply messages in OpenFlow
> >> * 1.0. */
> >> struct nicira10_stats_msg {
> >> @@ -85,14 +77,6 @@ struct nicira10_stats_msg {
> >> };
> >> OFP_ASSERT(sizeof(struct nicira10_stats_msg) == 24);
> >> 
> >> -/* Header for Nicira vendor stats request and reply messages in OpenFlow
> >> - * 1.1. */
> >> -struct nicira11_stats_msg {
> >> -    struct ofp11_vendor_stats_msg vsm; /* Vendor NX_VENDOR_ID. */
> >> -    ovs_be32 subtype;           /* One of NXST_* below. */
> >> -};
> >> -OFP_ASSERT(sizeof(struct nicira11_stats_msg) == 24);
> >> -
> >> /* Fields to use when hashing flows. */
> >> enum nx_hash_fields {
> >>    /* Ethernet source address (NXM_OF_ETH_SRC) only. */
> >> diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
> >> index 361150a..63d8b41 100644
> >> --- a/include/openflow/openflow-1.1.h
> >> +++ b/include/openflow/openflow-1.1.h
> >> @@ -391,17 +391,6 @@ struct ofp11_stats_msg {
> >> };
> >> OFP_ASSERT(sizeof(struct ofp11_stats_msg) == 16);
> >> 
> >> -/* Vendor extension stats message. */
> >> -struct ofp11_vendor_stats_msg {
> >> -    struct ofp11_stats_msg osm; /* Type OFPST_VENDOR. */
> >> -    ovs_be32 vendor;            /* Vendor ID:
> >> -                                 * - MSB 0: low-order bytes are IEEE OUI.
> >> -                                 * - MSB != 0: defined by OpenFlow
> >> -                                 *   consortium. */
> >> -    /* Followed by vendor-defined arbitrary additional data. */
> >> -};
> >> -OFP_ASSERT(sizeof(struct ofp11_vendor_stats_msg) == 20);
> >> -
> >> /* Stats request of type OFPST_FLOW. */
> >> struct ofp11_flow_stats_request {
> >>    uint8_t table_id;         /* ID of table to read (from ofp_table_stats),
> >> diff --git a/include/openflow/openflow-common.h b/include/openflow/openflow-common.h
> >> index da2b7a5..39c0e66 100644
> >> --- a/include/openflow/openflow-common.h
> >> +++ b/include/openflow/openflow-common.h
> >> @@ -422,17 +422,6 @@ struct ofp_hello_elem_header {
> >> };
> >> OFP_ASSERT(sizeof(struct ofp_hello_elem_header) == 4);
> >> 
> >> -/* Vendor extension. */
> >> -struct ofp_vendor_header {
> >> -    struct ofp_header header;   /* Type OFPT_VENDOR or OFPT_EXPERIMENTER. */
> >> -    ovs_be32 vendor;            /* Vendor ID:
> >> -                                 * - MSB 0: low-order bytes are IEEE OUI.
> >> -                                 * - MSB != 0: defined by OpenFlow
> >> -                                 *   consortium. */
> >> -    /* Vendor-defined arbitrary additional data. */
> >> -};
> >> -OFP_ASSERT(sizeof(struct ofp_vendor_header) == 12);
> >> -
> >> /* Table numbering. Tables can use any number up to OFPT_MAX. */
> >> enum ofp_table {
> >>    /* Last usable table number. */
> >> diff --git a/lib/ofp-msgs.c b/lib/ofp-msgs.c
> >> index 944ab33..fcfbb50 100644
> >> --- a/lib/ofp-msgs.c
> >> +++ b/lib/ofp-msgs.c
> >> @@ -35,6 +35,39 @@ VLOG_DEFINE_THIS_MODULE(ofp_msgs);
> >> #define OFPT11_STATS_REPLY 19
> >> #define OFPST_VENDOR 0xffff
> >> 
> >> +/* Vendor extension message. */
> >> +struct ofp_vendor_header {
> >> +    struct ofp_header header;   /* OFPT_VENDOR. */
> >> +    ovs_be32 vendor;            /* Vendor ID:
> >> +                                 * - MSB 0: low-order bytes are IEEE OUI.
> >> +                                 * - MSB != 0: defined by OpenFlow
> >> +                                 *   consortium. */
> >> +
> >> +    /* In theory everything after 'vendor' is vendor specific.  In practice,
> >> +     * the vendors we support put a 32-bit subtype here.  We'll change this
> >> +     * structure if we start adding support for other vendor formats. */
> >> +    ovs_be32 subtype;           /* Vendor-specific subtype. */
> >> +
> >> +    /* Followed by vendor-defined additional data. */
> >> +};
> >> +OFP_ASSERT(sizeof(struct ofp_vendor_header) == 16);
> >> +
> >> +/* Vendor extension stats message. */
> >> +struct ofp11_vendor_stats_msg {
> >> +    struct ofp11_stats_msg osm; /* Type OFPST_VENDOR. */
> >> +    ovs_be32 vendor;            /* Vendor ID:
> >> +                                 * - MSB 0: low-order bytes are IEEE OUI.
> >> +                                 * - MSB != 0: defined by OpenFlow
> >> +                                 *   consortium. */
> >> +
> >> +    /* In theory everything after 'vendor' is vendor specific.  In practice,
> >> +     * the vendors we support put a 32-bit subtype here.  We'll change this
> >> +     * structure if we start adding support for other vendor formats. */
> >> +    ovs_be32 subtype;           /* Vendor-specific subtype. */
> >> +
> >> +    /* Followed by vendor-defined additional data. */
> >> +};
> >> +OFP_ASSERT(sizeof(struct ofp11_vendor_stats_msg) == 24);
> >> /* A thin abstraction of OpenFlow headers:
> >> *
> >> *   - 'version' and 'type' come straight from struct ofp_header, so these are
> >> @@ -159,15 +192,8 @@ ofphdrs_decode(struct ofphdrs *hdrs,
> >> 
> >>        ovh = (const struct ofp_vendor_header *) oh;
> >>        hdrs->vendor = ntohl(ovh->vendor);
> >> -        if (hdrs->vendor == NX_VENDOR_ID) {
> >> -            /* Get Nicira message subtype (NXT_*). */
> >> -            const struct nicira_header *nh;
> >> -
> >> -            if (length < sizeof *nh) {
> >> -                return OFPERR_OFPBRC_BAD_LEN;
> >> -            }
> >> -            nh = (const struct nicira_header *) oh;
> >> -            hdrs->subtype = ntohl(nh->subtype);
> >> +        if (hdrs->vendor == NX_VENDOR_ID || hdrs->vendor == ONF_VENDOR_ID) {
> >> +            hdrs->subtype = ntohl(ovh->subtype);
> >>        } else {
> >>            log_bad_vendor(hdrs->vendor);
> >>            return OFPERR_OFPBRC_BAD_VENDOR;
> >> @@ -230,15 +256,9 @@ ofphdrs_decode(struct ofphdrs *hdrs,
> >> 
> >>            ovsm = (const struct ofp11_vendor_stats_msg *) oh;
> >>            hdrs->vendor = ntohl(ovsm->vendor);
> >> -            if (hdrs->vendor == NX_VENDOR_ID) {
> >> -                /* Get Nicira statistic type (NXST_*). */
> >> -                const struct nicira11_stats_msg *nsm;
> >> -
> >> -                if (length < sizeof *nsm) {
> >> -                    return OFPERR_OFPBRC_BAD_LEN;
> >> -                }
> >> -                nsm = (const struct nicira11_stats_msg *) oh;
> >> -                hdrs->subtype = ntohl(nsm->subtype);
> >> +            if (hdrs->vendor == NX_VENDOR_ID ||
> >> +                hdrs->vendor == ONF_VENDOR_ID) {
> >> +                hdrs->subtype = ntohl(ovsm->subtype);
> >>            } else {
> >>                log_bad_vendor(hdrs->vendor);
> >>                return OFPERR_OFPBRC_BAD_VENDOR;
> >> @@ -308,7 +328,7 @@ size_t
> >> ofphdrs_len(const struct ofphdrs *hdrs)
> >> {
> >>    if (hdrs->type == OFPT_VENDOR) {
> >> -        return sizeof(struct nicira_header);
> >> +        return sizeof(struct ofp_vendor_header);
> >>    }
> >> 
> >>    switch ((enum ofp_version) hdrs->version) {
> >> @@ -329,7 +349,7 @@ ofphdrs_len(const struct ofphdrs *hdrs)
> >>        if (hdrs->type == OFPT11_STATS_REQUEST ||
> >>            hdrs->type == OFPT11_STATS_REPLY) {
> >>            return (hdrs->stat == OFPST_VENDOR
> >> -                    ? sizeof(struct nicira11_stats_msg)
> >> +                    ? sizeof(struct ofp11_vendor_stats_msg)
> >>                    : sizeof(struct ofp11_stats_msg));
> >>        }
> >>        break;
> >> @@ -675,11 +695,10 @@ ofpraw_put__(enum ofpraw raw, uint8_t version, ovs_be32 xid,
> >>    oh->xid = xid;
> >> 
> >>    if (hdrs->type == OFPT_VENDOR) {
> >> -        struct nicira_header *nh = buf->header;
> >> +        struct ofp_vendor_header *ovh = buf->header;
> >> 
> >> -        ovs_assert(hdrs->vendor == NX_VENDOR_ID);
> >> -        nh->vendor = htonl(hdrs->vendor);
> >> -        nh->subtype = htonl(hdrs->subtype);
> >> +        ovh->vendor = htonl(hdrs->vendor);
> >> +        ovh->subtype = htonl(hdrs->subtype);
> >>    } else if (version == OFP10_VERSION
> >>               && (hdrs->type == OFPT10_STATS_REQUEST ||
> >>                   hdrs->type == OFPT10_STATS_REPLY)) {
> >> @@ -714,13 +733,7 @@ ofpraw_put__(enum ofpraw raw, uint8_t version, ovs_be32 xid,
> >>            struct ofp11_vendor_stats_msg *ovsm = buf->header;
> >> 
> >>            ovsm->vendor = htonl(hdrs->vendor);
> >> -            if (hdrs->vendor == NX_VENDOR_ID) {
> >> -                struct nicira11_stats_msg *nsm = buf->header;
> >> -
> >> -                nsm->subtype = htonl(hdrs->subtype);
> >> -            } else {
> >> -                OVS_NOT_REACHED();
> >> -            }
> >> +            ovsm->subtype = htonl(hdrs->subtype);
> >>        }
> >>    }
> >> }
> >> diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
> >> index a15efb6..38a2803 100644
> >> --- a/lib/ofp-msgs.h
> >> +++ b/lib/ofp-msgs.h
> >> @@ -60,9 +60,14 @@ struct ovs_list;
> >> *
> >> * where the syntax of each part is:
> >> *
> >> - *    - type: One of OFPT (standard OpenFlow message), OFPST (standard OpenFlow
> >> - *      statistics message), NXT (Nicira extension message), or NXST (Nicira
> >> - *      extension statistics message).
> >> + *    - type: One of the following:
> >> + *
> >> + *          * OFPT: standard OpenFlow message.
> >> + *          * OFPST: standard OpenFlow statistics or multipart message.
> >> + *          * NXT: Nicira extension message.
> >> + *          * NXST: Nicira extension statistics or multipart message.
> >> + *          * ONFT: Open Networking Foundation extension message.
> >> + *          * ONFST: Open Networking Foundation multipart message.
> >> *
> >> *      As new vendors implement extensions it will make sense to expand the
> >> *      dictionary of possible types.
> >> @@ -73,9 +78,9 @@ struct ovs_list;
> >> *    - number:
> >> *         For OFPT, the 'type' in struct ofp_header.
> >> *         For OFPST, the 'type' in struct ofp_stats_msg or ofp11_stats_msg.
> >> - *         For NXT, the 'subtype' in struct nicira_header.
> >> - *         For NXST, the 'subtype' in struct nicira10_stats_msg or
> >> - *           nicira11_stats_msg.
> >> + *         For NXT or ONFT, the 'subtype' in struct ofp_vendor_header.
> >> + *         For NXST or ONFST, the 'subtype' in an appropriate vendor stats
> >> + *         struct.
> >> *
> >> *    - arguments: The types of data that follow the OpenFlow headers (the
> >> *      message "body").  This can be "void" if the message has no body.
> >> -- 
> >> 2.1.3
> >> 
> > 
> 



More information about the dev mailing list