[ovs-dev] [PATCH v2 16/19] ovs-ofctl: Add 'bundle' command and unit testing.

[Jarno Rajahalme]

> On May 29, 2015, at 8:58 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> On Mon, May 18, 2015 at 04:10:25PM -0700, Jarno Rajahalme wrote:
>> The new ovs-ofctl 'bundle' command accepts files similar to
>> 'add-flows', but each line can optionally start with 'add', 'modify',
>> 'delete', 'modify_strict', or 'delete_strict' keyword, so that
>> arbitrary flow table modifications may be specified in a single file.
>> The modifications in the file are executed as a single transaction
>> using a OpenFlow 1.4 bundle.
>> 
>> Similarly, all existing ovs-ofctl flow mod commands now take an
>> optional '--bundle' argument, which executes the flow mods as a single
>> transaction.
>> 
>> OpenFlow 1.4 requires bundles to support at least flow and port mods.
>> This implementation does not yet support port mods in bundles.
>> 
>> Another restriction is that the atomic transactions are not yet
>> supported.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> I like the new command for doing a mix of operations from one file, but
> I suspect that this could be almost as useful without bundling.  Users
> could want to add and delete a mix of flows with any version of
> OpenFlow, and the bundle is really just a bonus.
> 
> So: what if you gave the command a more generic name, and then gave it a
> --bundle option like the rest of the commands that can be bundled?  (In
> fact, you could do this without adding a new command at all, instead
> allowing "add-flows" to support any command instead of just "add".  I
> think this would be fully backward compatible.)
> 

Great suggestion, done!

> In the manpage, usually we use bullets, with \(bu, instead of hyphens,
> for bulleted lists.

OK.

> 
> I think that ofctl_flow_mod__() could call bundle_flow_mod__() if
> 'bundle' is true, instead of having the caller deal with that.
> 

OK.

> When vconn_bundle_transact() finds an error and sends an
> OFPBCT_DISCARD_REQUEST, I suspect that the return value from
> vconn_bundle_control_transact() should not overwrite 'error', because
> then the previous error will be lost.
> 

Done. The discard error, if any should anyway be either reported or logged, so we now keep the original error code for the return value.

> I'm a little surprised to see the new "error_reporter" callback
> function.  Can you explain a little?
> 

Within a bundle we are now streaming the flow mods without waiting for a response after each flow mod, so error responses to earlier mods may arrive any time before the closing bundle control transaction. As there is no uniform way the users may wish to handle errors, this new callback allows the caller to do what they want. ovs-ofctl prints the error messages to stderr, some other callers may want to log them instead.

> In parse_ofp_str__(), I suspect that the "strict" versions won't ever be
> matched, because the shorter non-strict versions will be matched
> earlier.
> 

Actually, with strncmp() with the length of the token in the string, it is the reverse! The shorter non-strict version will not match a longer keyword, but if the order of the comparisons was changed, the longer strict version would match the shorter non-strict version in the string. (I tried this both ways.)

> Thanks for adding tests!
> 
> This will be really nice, thanks for implementing it.
> 
> (Now, to find out why there are three more commits...)


Will post a v3 with the rest of the series soon.

  Jarno