> On Jun 10, 2015, at 11:29 AM, Ben Pfaff <b...@nicira.com> wrote: > > On Tue, Jun 09, 2015 at 05:24:09PM -0700, Jarno Rajahalme wrote: >> 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+ is implicitly assumed when '--bundle' is >> specified. >> >> ovs-ofctl 'add-flow' and 'add-flows' commands now accept flow >> specifications that start with an optional 'add', 'modify', 'delete', >> 'modify_strict', or 'delete_strict' keyword, so that arbitrary flow >> table modifications may be specified. For backwards compatibility, a >> missing keyword is treated as an 'add'. With the new '--bundle' >> option all the modifications are executed as a single transaction >> using an OpenFlow 1.4 bundle. >> >> 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> > > "git am" says: > > Applying: ovs-ofctl: Add bundle support and unit testing. > /home/blp/nicira/ovs/.git/rebase-apply/patch:770: trailing whitespace. > vconn|DBG|unix: received: NXST_FLOW request: > warning: 1 line adds whitespace errors. >
This is a false positive, the whitespace is actually produced by OVS logging, and must be here as well for check to succeed. I spent a few minutes trying to hunt that trailing space down from the OVS code, but I did not find it, so I did not fix it. > In vconn_bundle_reply_validate(), I don't think the OpenFlow version > check is needed because vconns are only willing to receive the correct > version anyhow. > OK, removed. > In vconn_bundle_reply_validate(), I would usually write: > error = ofperr_decode_msg(oh, NULL); > return error; > as: > return ofperr_decode_msg(oh, NULL); > Changed. > I'd add a comment on vconn_bundle_control_transact(). In particular the > return value being an errno instead of an ofperr is useful > documentation. Also I think it can be EOF. > Added. > Above vconn_recv_error(), I'd wrap the comment into a single line. > Done. > It might be wise for vconn_recv_error() to loop, in the case where it > receives one error, so that it can report all of them if there are > multiple. > Added. > Callbacks are often inconvenient, so it might be better to handle > vconn_bundle_transact() make and return a list of errors, but it is OK > for now so I don't think it's worth changing. > I’ll keep it for now, we can change this later. Anyway, conn_bundle_add_msg() would also be changed to return a list of errors unrelated to the message being added. > It might be worth moving bundle_flow_mod__() a little earlier in > ovs-ofctl.c, to the point where it is currently just prototyped. > Done. > In bundle_transact(), I don't think the loop calling > ofpmsg_update_length() is necessary, because vconn does that for us: > > vconn_bundle_transact() calls > vconn_bundle_add_msg(), which calls > vconn_send_block(), which calls > vconn_send(), which calls > do_send(), which calls > ofpmsg_update_length(). > OK, I removed other calls to ofpmsg_update_length() in ovs-ofctl.c on the same grounds. > Acked-by: Ben Pfaff <b...@nicira.com <mailto:b...@nicira.com>> Thanks for the review! Pushed to master, Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
