Series now pushed to master, Jarno
> On Sep 14, 2016, at 2:57 PM, Jarno Rajahalme <ja...@ovn.org> wrote: > >> >> On Sep 13, 2016, at 5:09 PM, Ben Pfaff <b...@ovn.org <mailto:b...@ovn.org>> >> wrote: >> >> On Mon, Sep 12, 2016 at 01:52:43PM -0700, Jarno Rajahalme wrote: >>> Add support for OFPT_PACKET_OUT messages in bundles. >>> >>> While ovs-ofctl already has a packet-out command, we did not have a >>> string parser for it, as the parsing was done directly from command >>> line arguments. >>> >>> This patch adds the string parser for packet-out messages, and adds a >>> new ofctl/packet-out ovs-appctl command that can be used when >>> ovs-ofctl is used as a flow monitor. >>> >>> The new packet-out parser is further supported with the ovs-ofctl >>> bundle command, which allows bundles to mix flow mods, group mods and >>> packet-out messages. Also the packet-outs in bundles are only >>> executed if the whole bundle is successful. A failing packet-out >>> translation may also make the whole bundle to fail. >>> >>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org <mailto:ja...@ovn.org>> >> >> Should the existing packet-out command in ovs-ofctl also support this >> new syntax? It could distinguish it from the legacy syntax by checking >> the number of arguments (1 versus 4+). If so, perhaps we should >> deprecate the legacy syntax. > > OK. > >> I think that parse_ofp_packet_out_str__() has some memory leaks inside >> the while loop: if it detects than one error, then the earlier one is >> leaked. > > Fixed by adding “goto out;” after each time error is set to a non-NULL value, > rather than checking after multiple possible errors. > >> >> It's a little unusual that ofputil_encode_bundle_msgs() frees the >> bundled messages and that there's no separate way to do that without >> encoding. > > Added a separate ofputil_free_bundle_msgs(), and removed the freeing from the > ofputil_encode_bundle_msgs(). > >> >> I'm surprised that do_bundle_commit() potentially increases >> ofproto->tables_version more than once (in the loop labeled >> "4. Finish."). I would have guessed that it would do that once, to make >> visible everything in the bundle. >> > > We have no versioning for port_mods, which OpenFlow spec says need to be > supported in bundles. If the bundle does not include port mods, everything is > published as one new version, but if there are port mods, then the other > (flow, group, packet out) mods before and after each port mod are made > visible as one new version. I presume that if the caller is not specifying > the ‘ordered’ flag, we could first issue all the port mods and then commit > everything else as one version, but I have not wanted to deal with the > complications of ‘out-of-order’ handling of messages, as we would need to > detect potential conflicts among the messages. E.g., if one flow mod deletes > all the flows and another adds one, out-of-order execution could be > considered ‘conflicting’. Now we always execute everything in order, so the > semantics is clear. However, I’m not sure if we should still detect such > conflicts if the client does not specify the ‘ordered’ flag. > > Thanks a lot for the reviews, I’ll push the patches 6-13 in a moment (I > pushed patches 1-5 already yesterday). > > Jarno > >> Acked-by: Ben Pfaff <b...@ovn.org <mailto:b...@ovn.org>> _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
