> 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'm not sure I agree with this entirely. What if a client sends
multiple requests in a row very quickly. Doesn't it make sense to
process all of them at once in a single run loop instead of having to
go through an entire run loop per request? I suppose the problem with
the current code is that a client could arbitrarily fill the server's
backlog by sending lots of requests. It certainly would be easiest to
deal with this by not processing requests until the backlog is empty.
Alternatively we could set a maximum on the backlog and not process
requests when that value is hit. I'm not sure what the correct thing
to do is, do you have thoughts?
Ethan
>
> 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
