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]
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
