With the notes below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
(snip)
> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> index dad8707..bcc8758 100644
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016
> Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -179,11 +179,25 @@ struct nx_flow_mod_table_id {
> OFP_ASSERT(sizeof(struct nx_flow_mod_table_id) == 8);
>
> enum nx_packet_in_format {
> - NXPIF_OPENFLOW10 = 0, /* Standard OpenFlow 1.0 compatible. */
> - NXPIF_NXM = 1 /* Nicira Extended. */
> + NXPIF_STANDARD = 0, /* OFPT_PACKET_IN for this OpenFlow version.
> */
> + NXPIF_NXT_PACKET_IN = 1, /* NXT_PACKET_IN (since OVS v1.1). */
> + NXPIF_NXT_PACKET_IN2 = 2, /* NXT_PACKET_IN2 (since OVS v2.6). */
> };
>
> -/* NXT_SET_PACKET_IN_FORMAT request. */
> +/* NXT_SET_PACKET_IN_FORMAT request.
> + *
> + * For any given OpenFlow version, Open vSwitch supports multiple formats for
> + * "packet-in" messages. The default is always the standard format for the
> + * OpenFlow version in question, but NXT_SET_PACKET_IN_FORMAT can be used to
> + * set an alternative format.
> + *
> + * From OVS v1.1 to OVS v2.5, this request was only honored for OpenFlow 1.0.
> + * Requests to set format NXPIF_NXT_PACKET_IN were accepted for OF1.1+ but
> they
> + * had no effect. (Requests to set formats other than NXPIF_STANDARD or
> + * NXPIF_NXT_PACKET_IN2 were rejected with OFPBRC_EPERM.)
> + *
This should be NXPIF_NXT_PACKET_IN
> + * From OVS v2.6 onward, this request is honored for all OpenFlow versions.
> + */
> struct nx_set_packet_in_format {
> ovs_be32 format; /* One of NXPIF_*. */
> };
> @@ -246,6 +260,27 @@ struct nx_packet_in {
> };
> OFP_ASSERT(sizeof(struct nx_packet_in) == 24);
>
> +/* NXT_PACKET_IN2.
> + *
> + * NXT_PACKET_IN2 is conceptually similar to OFPT_PACKET_IN but it is
> expressed
> + * as an extensible set of properties instead of using a fixed structure.
> + *
> + * Added in Open vSwitch 2.6. */
> +enum nx_packet_in2_prop_type {
> + /* Packet. */
> + NXPINT_PACKET, /* Raw packet data. */
> + NXPINT_FULL_LEN, /* ovs_be32: Full packet len, if truncated.
> */
> + NXPINT_BUFFER_ID, /* ovs_be32: Buffer ID, if buffered. */
> +
> + /* Information about the flow that triggered the packet-in. */
> + NXPINT_TABLE_ID, /* uint8_t: Table ID. */
> + NXPINT_COOKIE, /* ovs_be64: Flow cookie (all-1s if none). */
Would leaving out the property in the none case work as well? I.e., do we need
the all-1s case?
> +
> + /* Other. */
> + NXPINT_REASON, /* uint8_t, one of OFPR_*. */
> + NXPINT_METADATA, /* NXM or OXM for metadata fields. */
> +};
> +
> /* Configures the "role" of the sending controller. The default role is:
> *
> * - Other (NX_ROLE_OTHER), which allows the controller access to all
> diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
> index a15efb6..8ce1f35 100644
> --- a/lib/ofp-msgs.h
> +++ b/lib/ofp-msgs.h
> @@ -152,6 +152,8 @@ enum ofpraw {
> OFPRAW_OFPT13_PACKET_IN,
> /* NXT 1.0+ (17): struct nx_packet_in, uint8_t[]. */
> OFPRAW_NXT_PACKET_IN,
> + /* NXT 1.0+ (30): uint8_t[]. */
As properties are 8-byte aligned (?), could this be expressed as uint8_t[8][],
or as uint64_t[]?
> + OFPRAW_NXT_PACKET_IN2,
>
> /* OFPT 1.0 (11): struct ofp10_flow_removed. */
> OFPRAW_OFPT10_FLOW_REMOVED,
> @@ -514,6 +516,7 @@ enum ofptype {
> * OFPRAW_OFPT11_PACKET_IN.
> * OFPRAW_OFPT12_PACKET_IN.
> * OFPRAW_OFPT13_PACKET_IN.
> + * OFPRAW_NXT_PACKET_IN2.
> * OFPRAW_NXT_PACKET_IN. */
> OFPTYPE_FLOW_REMOVED, /* OFPRAW_OFPT10_FLOW_REMOVED.
> * OFPRAW_OFPT11_FLOW_REMOVED.
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index d057915..d9dc0ad 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1125,8 +1125,9 @@ bool
> ofputil_packet_in_format_is_valid(enum nx_packet_in_format packet_in_format)
> {
> switch (packet_in_format) {
> - case NXPIF_OPENFLOW10:
> - case NXPIF_NXM:
> + case NXPIF_STANDARD:
> + case NXPIF_NXT_PACKET_IN:
> + case NXPIF_NXT_PACKET_IN2:
> return true;
> }
>
> @@ -1137,10 +1138,12 @@ const char *
> ofputil_packet_in_format_to_string(enum nx_packet_in_format packet_in_format)
> {
> switch (packet_in_format) {
> - case NXPIF_OPENFLOW10:
> - return "openflow10";
> - case NXPIF_NXM:
> - return "nxm";
> + case NXPIF_STANDARD:
> + return "standard";
> + case NXPIF_NXT_PACKET_IN:
> + return "nxt_packet_in";
> + case NXPIF_NXT_PACKET_IN2:
> + return "nxt_packet_in2";
> default:
> OVS_NOT_REACHED();
> }
> @@ -1149,8 +1152,12 @@ ofputil_packet_in_format_to_string(enum
> nx_packet_in_format packet_in_format)
> int
> ofputil_packet_in_format_from_string(const char *s)
> {
> - return (!strcmp(s, "openflow10") ? NXPIF_OPENFLOW10
> - : !strcmp(s, "nxm") ? NXPIF_NXM
> + return (!strcmp(s, "standard") || !strcmp(s, "openflow10")
> + ? NXPIF_STANDARD
> + : !strcmp(s, "nxt_packet_in") || !strcmp(s, "nxm")
> + ? NXPIF_NXT_PACKET_IN
> + : !strcmp(s, "nxt_packet_in2")
> + ? NXPIF_NXT_PACKET_IN2
> : -1);
> }
>
> @@ -3294,6 +3301,86 @@ ofputil_encode_flow_removed(const struct
> ofputil_flow_removed *fr,
> return msg;
> }
>
> +static enum ofperr
> +decode_nx_packet_in2(const struct ofp_header *oh,
> + struct ofputil_packet_in *pin,
> + size_t *total_len, uint32_t *buffer_id)
Is the caller responsible for initializing ‘*pin’ before making the call?
> +{
> + *total_len = 0;
> + *buffer_id = UINT32_MAX;
> +
> + struct ofpbuf properties;
> + ofpbuf_use_const(&properties, oh, ntohs(oh->length));
> + ofpraw_pull_assert(&properties);
> +
> + while (properties.size > 0) {
> + struct ofpbuf payload;
> + uint64_t type;
> +
> + enum ofperr error = ofpprop_pull(&properties, &payload, &type);
> + if (error) {
> + return error;
> + }
> +
> + switch (type) {
> + case NXPINT_PACKET:
> + pin->packet = payload.msg;
> + pin->len = ofpbuf_msgsize(&payload);
> + break;
> +
> + case NXPINT_FULL_LEN: {
> + uint32_t u32;
> + error = ofpprop_parse_u32(&payload, &u32);
> + *total_len = u32;
> + break;
> + }
> +
> + case NXPINT_BUFFER_ID:
> + error = ofpprop_parse_u32(&payload, buffer_id);
> + break;
> +
> + case NXPINT_TABLE_ID:
> + error = ofpprop_parse_u8(&payload, &pin->table_id);
> + break;
> +
> + case NXPINT_COOKIE:
> + error = ofpprop_parse_be64(&payload, &pin->cookie);
> + break;
We should fill in all-1s if this property is missing?
> +
> + case NXPINT_REASON: {
> + uint8_t reason;
> + error = ofpprop_parse_u8(&payload, &reason);
> + pin->reason = reason;
> + break;
> + }
> +
> + case NXPINT_METADATA:
> + error = oxm_decode_match(payload.msg, ofpbuf_msgsize(&payload),
> + &pin->flow_metadata);
> + break;
> +
> + default:
> + error = OFPPROP_UNKNOWN(false, "NX_PACKET_IN2", type);
> + break;
Could unknown properties be silently ignored? Or should we define a range of
properties that could be silently ignored if unknown?
> + }
> + if (error) {
> + return error;
> + }
> + }
> +
> + if (!pin->len) {
> + VLOG_WARN_RL(&bad_ofmsg_rl, "NXT_PACKET_IN2 lacks packet");
> + return OFPERR_OFPBRC_BAD_LEN;
> + } else if (!*total_len) {
> + *total_len = pin->len;
> + } else if (*total_len < pin->len) {
> + VLOG_WARN_RL(&bad_ofmsg_rl, "NXT_PACKET_IN2 claimed full_len < len");
> + return OFPERR_OFPBRC_BAD_LEN;
> + }
> +
> + return 0;
> +}
> +
> /* Decodes the packet-in message starting at 'oh' into '*pin'. Populates
> * 'pin->packet' and 'pin->len' with the part of the packet actually included
> * in the message, and '*total_len' with the original length of the packet
> @@ -3394,6 +3481,8 @@ ofputil_decode_packet_in(const struct ofp_header *oh,
>
> pin->packet = b.data;
> pin->len = b.size;
> + } else if (raw == OFPRAW_NXT_PACKET_IN2) {
> + return decode_nx_packet_in2(oh, pin, total_len, buffer_id);
> } else {
> OVS_NOT_REACHED();
> }
> @@ -3448,14 +3537,14 @@ ofputil_encode_ofp10_packet_in(const struct
> ofputil_packet_in *pin,
>
> static struct ofpbuf *
> ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
> - uint32_t buffer_id)
> + enum ofp_version version, uint32_t buffer_id)
indentation wrong here?
(snip)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
