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 <ja...@ovn.org> wrote:
> >
> > I was able to implement the Extension 230 on top of these, so looking good
> > :-)
> >
> > Acked-by: Jarno Rajahalme <ja...@ovn.org>
> >
> >> On Jan 27, 2016, at 3:50 PM, Ben Pfaff <b...@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 <ja...@ovn.org>
> >> Signed-off-by: Ben Pfaff <b...@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
> >>
> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
