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.
I'd probably use this:
OFPRAW_OFPT11_FLOW_REMOVED,
/* OFPT 1.1+ (11): struct ofp11_flow_removed, uint8_t[8][]. */
> 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
