[ovs-dev] [PATCH 2/2] unixctl: New JSON RPC back-end.

Ethan Jackson ethan at nicira.com
Mon Feb 27 21:30:13 UTC 2012


> 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");
>  }

I hadn't realized this fixes a bug as I wrote the patch, I've sent out
a new patch.

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

My thinking was that once I've sent the request, I need to destroy the
'conn' object. and now the caller has this dangling reference.
However, the more I think about it, the less valid this argument is
because I already make assertions preventing a caller from making a
reply twice.  Perhaps simply sending the request, and documenting that
only one reply may be made per callback invocation is sufficient.  It
certainly would simplify the code significantly.  I'll look into
changing it.

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

I was modeling my code mostly of unixctl server which is why I ended
up with the code I did.  This shouldn't be too hard to simplify, let
me have another crack at it and I'll send out an incremental.

> 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?

Yes this can't happen.  I figured if it happens it means there's a bug
and it's worth at least logging.  Alternatively I could do a
NOT_REACHED().  Or perhaps I'm being too defensive and ignoring the
possibility is preferable.

Ethan



More information about the dev mailing list