[ovs-dev] [PATCH v3 13/13] ofproto: Support packet_outs in bundles.

[Jarno Rajahalme]

> On Sep 13, 2016, at 5:09 PM, Ben Pfaff <blp at 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 <jarno at 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 <blp at ovn.org <mailto:blp at ovn.org>>