On Thu, Jul 19, 2012 at 11:23:30PM -0700, Ben Pfaff wrote:
> On Fri, Jul 20, 2012 at 03:05:17PM +0900, Simon Horman wrote:
> > On Thu, Jul 05, 2012 at 11:12:29PM -0700, Ben Pfaff wrote:
> > > OpenFlow headers are not as uniform as they could be, with size,
> > > alignment,
> > > and numbering changes from one version to another and across varieties
> > > (e.g. ordinary messages vs. "stats" messages). Until now the Open vSwitch
> > > internal APIs haven't done a good job of abstracting those differences in
> > > header formats. This commit changes that; from this commit forward very
> > > little code actually needs to understand the header format or numbering.
> > > Instead, it can just encode or decode, or pull or put, the header using
> > > a more abstract API using the ofpraw_, ofptype_, and other APIs in the
> > > new ofp-msgs module.
> > >
> > > Signed-off-by: Ben Pfaff <b...@nicira.com>
>
> ...
>
> > > + /* OFPT 1.0 (10): struct ofp_packet_in up to data, uint8_t[]. */
> > > + OFPRAW_OFPT10_PACKET_IN,
> > > + /* OFPT 1.1 (11): struct ofp11_packet_in up to data, uint8_t[]. */
> > > + OFPRAW_OFPT11_PACKET_IN,
> >
> > I believe that OFPRAW_OFPT11_PACKET_IN's number should be 10.
>
> Thank you. Fixed.
>
> > > + /* NXT 1.0+ (17): struct nx_packet_in, uint8_t[]. */
> > > + OFPRAW_NXT_PACKET_IN,
> > > +
> > > + /* OFPT 1.0 (11): struct ofp_flow_removed. */
> > > + OFPRAW_OFPT10_FLOW_REMOVED,
> > > + /* NXT 1.0+ (14): struct nx_flow_removed, uint8_t[8][]. */
> > > + OFPRAW_NXT_FLOW_REMOVED,
> >
> > I'm unsure how important the arguments are.
> >
> > For example, in the case of Flow Removed, OF1.1 has a 40byte
> > struct ofp11_flow_removed followed by an 88 byte struct ofp11_match.
> > OF1.2 has a 40 byte struct ofp13_flow_removed, followed by a 4 byte
> > ofp12_match, followed by uing8_t[], followed by a pad to ensure 8 byte
> > alignment.
> >
> > In this case should both be covered by something like this?
> >
> > OFPRAW_OFPT11_FLOW_REMOVED,
> > /* OFPT 1.2 (11): struct ofp11_flow_removed, uint8_t[8][]. */
>
> OF1.1 and OF1.2 don't really differ here, because ofp11_flow_removed and
> ofp12_flow_removed are the same structure (well ofp12_flow_removed has
> one tiny difference, but you can use the same structure just fine), and
> the match that follows is covered by a "struct ofp_match_header" with
> whatever comes after it.
>
> So, that comes down to the parser code not really caring to distinguish
> between the two anyhow, which means that the value of having two
> different OFPRAW_ constants for them is minimal at best.
Thanks, that answers my question.
> I'd probably use this:
>
> OFPRAW_OFPT11_FLOW_REMOVED,
> /* OFPT 1.1+ (11): struct ofp11_flow_removed, uint8_t[8][]. */
Thanks, I'll use that.
> > Or are two separate entries needed?
> >
> > /* OFPT 1.1 (11): struct ofp11_flow_removed, struct ofp11_match[]. */
> > OFPRAW_OFPT11_FLOW_REMOVED,
> > /* OFPT 1.2 (11): struct ofp12_flow_removed, uint8_t[8][]. */
> > OFPRAW_OFPT12_FLOW_REMOVED,
>
> Not much value in that (as I said above).
>
> > In the case of the latter I see.
> >
> > ./lib/ofp-msgs.h:138: Duplicate message definition for (2, 11, 0, 0, 0).
> > ./lib/ofp-msgs.h:135: Here is the location of the previous definition.
>
> That's just a bug in extract-ofp-msgs. Sorry. Here's an incremental
> fix (it fixes another few bugs I noticed at the same time).
>
> diff --git a/build-aux/extract-ofp-msgs b/build-aux/extract-ofp-msgs
> index c506408..fe92dae 100755
> --- a/build-aux/extract-ofp-msgs
> +++ b/build-aux/extract-ofp-msgs
> @@ -22,8 +22,8 @@ OFPST_VENDOR = 0xffff
>
> version_map = {"1.0": (OFP10_VERSION, OFP10_VERSION),
> "1.1": (OFP11_VERSION, OFP11_VERSION),
> - "1.2": (OFP11_VERSION, OFP11_VERSION),
> - "1.3": (OFP11_VERSION, OFP11_VERSION),
> + "1.2": (OFP12_VERSION, OFP12_VERSION),
> + "1.3": (OFP13_VERSION, OFP13_VERSION),
> "1.0+": (OFP10_VERSION, OFP13_VERSION),
> "1.1+": (OFP11_VERSION, OFP13_VERSION),
> "1.2+": (OFP12_VERSION, OFP13_VERSION),
> @@ -284,6 +284,9 @@ def extract_ofp_msgs(output_file_name):
> output.append("static struct raw_info raw_infos[] = {")
> for raw in all_raws_order:
> r = all_raws[raw]
> + if "ofptype" not in r:
> + error("%s: no defined OFPTYPE_" % raw)
> + continue
> output.append(" {")
> output.append(" %s_instances," % raw.lower())
> output.append(" %d, %d," % (r["min_version"],
> r["max_version"]))
> @@ -321,6 +324,10 @@ def extract_ofp_msgs(output_file_name):
> fatal("%s has no corresponding request"
> % r["human_name"])
> output.append("};")
> +
> + if n_errors:
> + sys.exit(1)
> +
> return output
>
>
> I folded this in.
Thanks.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
