On Fri, Jun 13, 2014 at 02:58:32PM -0700, Daniele Di Proietto wrote:
> This commit intruduces a new appctl command: dpif/dpctl
s/intruduces/introduces/
> It's needed to interact with userspace datapaths (dpif-netdev), because the
> ovs-dpctl command runs in a separate process and cannot see the userspace
> datapaths inside vswitchd.
>
> This change moves most of the code of utilities/ovs-dpctl.c in lib/dpctl.c.
>
> Both the ovs-dpctl command and the ovs-appctl dpif/dpctl command make calls to
> lib/dpctl.c functions, to interact with datapaths.
>
> Signed-off-by: Daniele Di Proietto <ddiproie...@vmware.com>
I think that all of the commentary below should be moved into the
commit message:
> The code from utilities/ovs-dpctl.c has been moved to lib/dpctl.c and has been
> changed for different reasons:
> - An exit() call in the old code made perfectly sense. Now (since the code
> can be run inside vswitchd) it would terminate the daemon. Same
> reasoning
> can be applied to ovs_fatal-*() calls.
> - The lib/dpctl.c _should_ not leak memory.
> - All the print* have been replaced with a function pointer provided by
> the
> caller, since this code can be run in the ovs-dpctl process (in which
> case we need to print to stdout) or in response to a unixctl request
> (and
> in this case we need to send everything through a socket, using JSON
> encapsulation).
>
> The syntax is
> ovs-appctl dpif/dpctl (COMMAND AND PARAMETERS)
> while the ovs-dpctl syntax (which _should_ remain the same after this change)
> is
> ovs-dpctl [OPTIONS] (COMMAND AND PARAMETERS)
>
> The appctl version doesn't accept options (most of them do not make sense for
> an appctl command anyway).
>
> Again, this should leave the ovs-dpctl behaviour unchanged.
I see 3 calls to va_start() without matching calls to va_end().
I think that dpctl_error() might be more readable if it composed a
single "struct ds" instead of using multiple calls to dpctl_puts().
The run() function made some sense when it was able to encapsulate the
whole behavior. Now, I think that it would be better (easier to
follow) if code like this:
x = run(dpctl_p, function(...), "some comment");
if (x) {
return;
}
were rewritten as:
x = function(...);
if (x) {
dpctl_error(dpctl_p, "some comment");
return x;
}
On the same note, does it make sense to have the handlers just return
the errno value, as an int, rather than store it into the structure?
I lean toward using the return value.
I don't understand the semantics of the 'error' member in struct
dpctl_params. Sometimes it is set to an errno value, otherwise just
to 1 to indicate an error. Some places it is set twice, once by
dpctl_error() and then right afterward to 1.
We are starting to accumulate multiple copies of the logic in
dpctl_run_command(). In the long run, I would like to consolidate
them as much as we can. It is probably OK for now.
I think that the definition of struct dpctl_command can be moved into
dpctl.c, since only dpctl.c uses it.
I think that dpif.c should try to parse the options. I think it would
be OK to just look for them before a command name, without the
generality that getopt_long() supports.
In dpctl.h, we usually use 'aux' instead of 'userdata' for
user-supplied arguments.
I'd be happier if each of the commands were available as a separate
unixctl command, instead of being made subcommands behind a single
command. Can you try to think of a nice way to do this?
The new commands should be documented in the ovs-vswitchd manpage.
Ideally, this could be done through a .man file included by both
ovs-vswitchd.8.in and utilities/ovs-dpctl.8.in (using a .so
directive).
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
