[ovs-dev] [flow-stats 05/14] unixctl: Implement quoting.

[Ethan Jackson]

All your comments sound reasonable.

Ethan

On Wed, Dec 14, 2011 at 09:25, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Dec 13, 2011 at 03:18:17PM -0800, Ethan Jackson wrote:
>> Why are min and max args 'int's instead of 'size_t's or 'unsigned'?
>
> I was drawing an analogy to main(), which has an "int" for argc.
>
>> In ofproto/trace, you have a case which expects argc == 6, but you
>> only allow a maximum of 4 (+ 1 = 5) arguments when you register.  It
>> looks like you are leaving off the priority field in the usage of the
>> register call.
>
> Good point, both fixed:
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 23b3369..4661d2b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5787,8 +5787,8 @@ ofproto_dpif_unixctl_init(void)
>
>     unixctl_command_register(
>         "ofproto/trace",
> -        "bridge {tun_id in_port packet | odp_flow [-generate]}",
> -        2, 4, ofproto_unixctl_trace, NULL);
> +        "bridge {priority tun_id in_port packet | odp_flow [-generate]}",
> +        2, 5, ofproto_unixctl_trace, NULL);
>     unixctl_command_register("fdb/flush", "bridge", 1, 1,
>                              ofproto_unixctl_fdb_flush, NULL);
>     unixctl_command_register("fdb/show", "bridge", 1, 1,
>
>> The argument parsing of ofproto/trace is pretty confusing in general.
>> I wonder if it would be worth unit testing all of the possible code
>> paths beyond what we're already doing for the ofproto-dpif unit tests.
>
> I agree that that is a good idea.
>
>> > @@ -803,11 +803,8 @@ netdev_linux_recv(struct netdev *netdev_, void *data, size_t size)
>> >
>> >     for (;;) {
>> >         ssize_t retval = recv(netdev->fd, data, size, MSG_TRUNC);
>> > -        if (retval > size) {
>> > -            /* Received packet was longer than supplied buffer. */
>> > -            return -EMSGSIZE;
>> > -        } else if (retval >= 0) {
>> > -            return retval;
>> > +        if (retval >= 0) {
>> > +            return retval <= size ? retval : -EMSGSIZE;
>>
>> This looks like a rebasing error to me.  Was it intended for this patch?
>
> You are right, that's a mistake.  I have now folded it into
> "netdev-linux: Report error for truncated packets on receive."
>
>> > +    for (i = 0; i < n_stress_options; i++) {
>> > +        struct stress_option *option = stress_options[i];
>> > +        if (!strcmp(option_name, option->name)) {
>> > +            unsigned int period = strtoul(option_val, NULL, 0);
>> > +            bool random = !strcmp(argv[3], "random");
>>
>> argv[3] could be NULL here.
>
> Good point, fixed:
>
> diff --git a/lib/stress.c b/lib/stress.c
> index 0fdc79a..7247b3b 100644
> --- a/lib/stress.c
> +++ b/lib/stress.c
> @@ -190,7 +190,7 @@ stress_unixctl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>         struct stress_option *option = stress_options[i];
>         if (!strcmp(option_name, option->name)) {
>             unsigned int period = strtoul(option_val, NULL, 0);
> -            bool random = !strcmp(argv[3], "random");
> +            bool random = argc >= 4 && !strcmp(argv[3], "random");
>
>             stress_set(option, period, random);
>             code = 200;
>
>> > +    unixctl_command_register(
>> > +        "vlog/set", "{module[:facility[:level]] | PATTERN:facility:pattern}",
>>
>> I might be inclined to add a ... to the end of the usage argument.
>> However, I don't know what standard practice for this sort of thing
>> is.
>
> Sure, I added the ..., thank you.