[ovs-dev] [PATCH] dpif: add ovs-appctl dpif/dpctl to talk to dpif-netdev

Daniele Di Proietto ddiproietto at vmware.com
Fri Jul 18 00:35:27 UTC 2014


Thanks Ben the review, and sorry for the delay.
I’m about to drop a v2 with your suggestions.

On Jun 23, 2014, at 2:24 PM, Ben Pfaff <blp at nicira.com> wrote:

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

sorry, fixed

>> 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 <ddiproietto at vmware.com>
> 
> I think that all of the commentary below should be moved into the
> commit message:
> 

I’ll do that.

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

fixed, thanks

> I think that dpctl_error() might be more readable if it composed a
> single "struct ds" instead of using multiple calls to dpctl_puts().
> 

this makes sense. I’ll do that.

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

It is definitely better, thanks.

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

You’re right, it’s confusing. I removed the ‘error’ fields and used return
values instead.

> 
> 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’ve decided to move the whole unixctl handler to dpctl.c. Let me know
if you think it’s a good idea

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

I wrote a small parser that should be enough for our purposes.

> 
> In dpctl.h, we usually use 'aux' instead of 'userdata' for
> user-supplied arguments.

ok, I’ll change that

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

You’re right, it’s better. 

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

I didn’t know that, sorry. I added them to the manage with the .so
directive and used a macro to deal with the different position of the
options

> Thanks,
> 

Thank you very much, every comment made a lot of sense.

Daniele


More information about the dev mailing list