[ovs-dev] [PATCH 3/3] ovs-benchmark: New utility.

[Ben Pfaff]

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);
        }
    }