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.

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.

In vconn_bundle_reply_validate(), I would usually write:
        error = ofperr_decode_msg(oh, NULL);
        return error;
as:
        return ofperr_decode_msg(oh, NULL);

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.

Above vconn_recv_error(), I'd wrap the comment into a single line.

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.

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.

It might be worth moving bundle_flow_mod__() a little earlier in
ovs-ofctl.c, to the point where it is currently just prototyped.

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().

Acked-by: Ben Pfaff <b...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to