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

Ethan Jackson ethan at nicira.com
Tue Dec 13 23:18:17 UTC 2011


This looks like a huge win in terms of code cleanup.  Comments below.

Why are min and max args 'int's instead of 'size_t's or 'unsigned'?

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.

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.

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

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


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

Ethan



More information about the dev mailing list