On Thu, Feb 16, 2012 at 04:57:17PM -0800, Ethan Jackson wrote:
> The unixctl library had used the vde2 management protocol since the
> early days of Open vSwitch. As Open vSwitch has matured, several
> Python daemons have been added to the code base which would benefit
> from a unixctl implementations. Instead of implementing the old
> unixctl protocol in Python, this patch changes unixctl to use JSON
> RPC for which we already have an implementation in both Python and
> C. Future patches will need to implement a unixctl library in
> Python on top of JSON RPC.
>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
This looks good. I have a few nits to pick.
In enable_slave() in lib/bond.c, I think that the following is
actually a bug fix. If so, can you separate out the bug fix from the
wholesale conversion, so that we can commit it to old release
branches?
bond_enable_slave(slave, enable, &bond->unixctl_tags);
- unixctl_command_reply(conn, 501, enable ? "enabled" : "disabled");
+ unixctl_command_reply(conn, enable ? "enabled" : "disabled");
}
static void
I don't understand why unixctl_command_reply__() stores the reply
message in a member of unixctl_conn instead of immediately sending
it with jsonrpc_send().
I would argue that run_connection() should only read a new request
if the connection is not backlogged (this is already tested in
unixctl_server_wait() so maybe that was your intention all along?).
I don't think that run_connection() really needs a loop. One try
should be enough. (I think I must have overthought the logic in
there.) Ditto for the loop around pstream_accept() in
unixctl_server_run().
There's a doubled space between "conn," and "msg" below:
@@ -452,7 +452,7 @@ vlog_unixctl_list(struct unixctl_conn *conn, int argc
OVS_UNUSED,
const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
{
char *msg = vlog_get_levels();
- unixctl_command_reply(conn, 200, msg);
+ unixctl_command_reply(conn, msg);
free(msg);
}
There's a corner case in ovs-appctl that prints "server did not return
a result or an error". I don't think it's possible to trigger this
case, because I think that unixctl_client_transact() won't ever return
such a combination. What do you think?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
