[ovs-dev] [of1.1 draft v2 2/7] Introduce ofpacts, an abstraction of OpenFlow actions.

[Simon Horman]

On Fri, Feb 03, 2012 at 12:43:50PM -0800, Ben Pfaff wrote:
> OpenFlow actions have always been somewhat awkward to handle.
> Moreover, over time we've started creating actions that require more
> complicated parsing.  When we maintain those actions internally in
> their wire format, we end up parsing them multiple times, whenever
> we have to look at the set of actions.
> 
> When we add support for OpenFlow 1.1 or later protocols, the situation
> will get worse, because these newer protocols support many of the same
> actions but with different representations.  It becomes unrealistic to
> handle each protocol in its wire format.
> 
> This commit adopts a new strategy, by converting OpenFlow actions into
> an internal form from the wire format when they are read, and converting
> them back to the wire format when flows are dumped.  I believe that this
> will be more maintainable over time.
> 
> The following improvements are necessary before this commit is ready:
> 
>   - The current commit formats internal actions as Netlink.  This is
>     an imperfect choice because Netlink attributes are only 4-byte
>     aligned, which makes it awkward to include 64-bit integers and
>     pointers inside actions.  It would be nice to figure out a way to
>     do this, either by adding a way to 64-bit align Netlink attributes
>     or to use a different framing format.
> 
>   - Add comments in various places.
> 
>   - The new functions in netlink, meta-flow, and ofp-util might be easier
>     to review as separate commits.
> 
>   - A few minor points marked with XXX.

[snip]

> diff --git a/lib/bundle.c b/lib/bundle.c
> index 733d79a..9149c1c 100644
> --- a/lib/bundle.c
> +++ b/lib/bundle.c

[snip]

> @@ -290,15 +328,15 @@ bundle_parse(struct ofpbuf *b, const char *s)
>      slave_type = strtok_r(NULL, ", ", &save_ptr);
>      slave_delim = strtok_r(NULL, ": ", &save_ptr);

Not strictly related to this patch, but it is unclear to me how it
is ensured that the strtok_r calls above (and others above them)
will not return NULL and it seems that bundle_parse__() will be
rather unhappy if any of the values are Null.

>  
> -    bundle_parse__(b, s, &save_ptr, fields, basis, algorithm, slave_type, NULL,
> -                   slave_delim);
> +    bundle_parse__(s, &save_ptr, fields, basis, algorithm, slave_type, NULL,
> +                   slave_delim, ofpacts);
>      free(tokstr);
>  }
>  
>  /* Converts a bundle_load action string contained in 's' to an nx_action_bundle
>   * and stores it in 'b'.  Sets 'b''s l2 pointer to NULL. */
>  void
> -bundle_parse_load(struct ofpbuf *b, const char *s)
> +bundle_parse_load(const char *s, struct ofpbuf *ofpacts)
>  {
>      char *fields, *basis, *algorithm, *slave_type, *dst, *slave_delim;
>      char *tokstr, *save_ptr;
> @@ -312,22 +350,22 @@ bundle_parse_load(struct ofpbuf *b, const char *s)
>      dst = strtok_r(NULL, ", ", &save_ptr);
>      slave_delim = strtok_r(NULL, ": ", &save_ptr);
>  
> -    bundle_parse__(b, s, &save_ptr, fields, basis, algorithm, slave_type, dst,
> -                   slave_delim);
> +    bundle_parse__(s, &save_ptr, fields, basis, algorithm, slave_type, dst,
> +                   slave_delim, ofpacts);

Here too.

>      free(tokstr);
>  }

[snip]