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 <b...@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 <ddiproie...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
