[ovs-dev] [of1.1 v5 1/5] ofp-msgs: New approach to encoding and decoding OpenFlow headers.

[Simon Horman]

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 <blp at 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.