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 <jrajaha...@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.) In the manpage, usually we use bullets, with \(bu, instead of hyphens, for bulleted lists. I think that ofctl_flow_mod__() could call bundle_flow_mod__() if 'bundle' is true, instead of having the caller deal with that. 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. I'm a little surprised to see the new "error_reporter" callback function. Can you explain a little? 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. Thanks for adding tests! This will be really nice, thanks for implementing it. (Now, to find out why there are three more commits...) _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
