[ovs-dev] [of1.1 rollup 12/20] Introduce ofpacts, an abstraction of OpenFlow actions.

[Simon Horman]

On Thu, Jun 14, 2012 at 09:58:08AM +0900, Simon Horman wrote:
> On Tue, Jun 12, 2012 at 12:32:16AM -0700, 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.
> > 
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> 
> This is a pretty big patch and I'm not going to pretend to have reviewed it
> completely. But I did notice one thing.
> 
> [snip]
> 
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index 7413455..630e457 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> 
> [snip]
> 
> > @@ -1232,22 +1232,23 @@ do_probe(int argc OVS_UNUSED, char *argv[])
> >  static void
> >  do_packet_out(int argc, char *argv[])
> >  {
> > +    enum ofputil_protocol protocol;
> >      struct ofputil_packet_out po;
> > -    struct ofpbuf actions;
> > +    struct ofpbuf ofpacts;
> >      struct vconn *vconn;
> >      int i;
> >  
> > -    ofpbuf_init(&actions, sizeof(union ofp_action));
> > -    parse_ofp_actions(argv[3], &actions);
> > +    ofpbuf_init(&ofpacts, 64);
> > +    parse_ofpacts(argv[3], &ofpacts);
> >  
> >      po.buffer_id = UINT32_MAX;
> >      po.in_port = (!strcasecmp(argv[2], "none") ? OFPP_NONE
> >                    : !strcasecmp(argv[2], "local") ? OFPP_LOCAL
> >                    : str_to_port_no(argv[1], argv[2]));
> > -    po.actions = actions.data;
> > -    po.n_actions = actions.size / sizeof(union ofp_action);
> > +    po.ofpacts = ofpacts.data;
> > +    po.ofpacts_len = ofpacts.size;
> >  
> > -    open_vconn(argv[1], &vconn);
> > +    protocol = open_vconn(argv[1], &vconn);
> >      for (i = 4; i < argc; i++) {
> >          struct ofpbuf *packet, *opo;
> >          const char *error_msg;
> 
> GCC tells me that protocol is set by not used in do_packet_out().
> Perhaps it could be removed?

Actually, scratch that.

I think that protocol will need to be passed to ofputil_encode_packet_out()
which is called inside the for loop above.

Actually, I have a local change to add protocol to do_packet_out()
for that reason. I had forgotten about that.