Sounds good. Thanks.
On Wed, Jul 27, 2011 at 16:16, Ben Pfaff <[email protected]> wrote: > On Wed, Jul 27, 2011 at 01:09:42PM -0700, Ethan Jackson wrote: >> There is some trailing whitespace in the man page. > > Oops, deleted. > >> I find it marginally easier to read do-while loops when they are >> followed by a newline. If you disagree then leave it. > > The ones in ovs-benchmark.c look OK to me. The code just after the loop > is closely related to the code inside it. > >> > + ? ?fds = xmalloc((local_max_port - local_min_port) * sizeof *fds); >> >> I think you want (1 + local_max_port - local_min_port). > > Thanks. > >> > + ? ? ? ? ? ? ? ?if (newfd >= 0) { >> > + ? ? ? ? ? ? ? ? ? ?close(newfd); >> > + ? ? ? ? ? ? ? ?} else if (errno != EAGAIN) { >> > + ? ? ? ? ? ? ? ? ? ?ovs_fatal(errno, "accept failed"); >> > + ? ? ? ? ? ? ? ?} >> > + >> Redundant newline. > > Thanks, I deleted it. > >> > +static void >> > +next_ports(unsigned short int *local_port, unsigned short int >> > *remote_port) >> > +{ >> > + ? ?if ((*local_port)++ == local_max_port) { >> > + ? ? ? ?*local_port = local_min_port; >> > + ? ? ? ?if ((*remote_port)++ == remote_max_port) { >> > + ? ? ? ? ? ?*remote_port = remote_min_port; >> > + ? ? ? ?} >> > + ? ?} >> > +} >> >> I found this function difficult to read. Maybe something like this >> would be more obviously correct? >> static void >> next_ports(unsigned short int *local_port, unsigned short int *remote_port) >> { >> if (*local_port >= local_max_port) { >> *local_port = local_min_port; >> if (*remote_port >= remote_max_port) { >> *remote_port = remote_min_port; >> } else { >> (*remote_port)++; >> } >> } else { >> (*local_port)++; >> } >> } > > I changed it to the following: > > /* Increments '*value' within the range 'min...max' inclusive. Returns > true > * if '*value' wraps around to 'min', otherwise false. */ > static bool > increment(unsigned short int *value, > unsigned short int min, unsigned short int max) > { > if (*value < max) { > ++*value; > return false; > } else { > *value = min; > return true; > } > } > > static void > next_ports(unsigned short int *local_port, unsigned short int *remote_port) > { > if (increment(local_port, local_min_port, local_max_port)) { > increment(remote_port, remote_min_port, remote_max_port); > } > } > > > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
