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
