All your comments sound reasonable.
Ethan
On Wed, Dec 14, 2011 at 09:25, Ben Pfaff <b...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
