Re: [Qemu-devel] [RFC][PATCH v3 11/21] virtproxy: add handler for control packet

[Michael Roth]

On 11/18/2010 05:35 AM, Jes Sorensen wrote:
> On 11/16/10 02:16, Michael Roth wrote:
> > Process control packets coming in over the channel. This entails setting
> > up/tearing down connections to local services initiated from the other
> > end of the channel.
> >
> > Signed-off-by: Michael Roth<address@hidden>
> > ---
> >   virtproxy.c |  154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 files changed, 154 insertions(+), 0 deletions(-)
>
> [snip]
>
> > +
> > +        qemu_opts_print(iforward->socket_opts, NULL);
> > +        if (qemu_opt_get(iforward->socket_opts, "host") != NULL) {
> > +            server_fd = inet_connect_opts(iforward->socket_opts);
> > +        } else if (qemu_opt_get(iforward->socket_opts, "path") != NULL) {
> > +            server_fd = unix_connect_opts(iforward->socket_opts);
> > +        } else {
> > +            LOG("unable to find listening socket host/addr info");
> > +            return -1;
> > +        }
>
> This patch is a perfect example of why -1 as an error message is suboptimal.
>
> > +        closesocket(fd);
> > +        vp_set_fd_handler(fd, NULL, NULL, conn);
> > +        QLIST_REMOVE(conn, next);
> > +        qemu_free(conn);
> > +        break;
> > +    }
> > +    }
>
> You should never have two closing braces in the same column like this -
> something is wrong with the formatting.

That's from using a block for the case
switch () {
case: {
    ...
}
}

Alternative is to indent the "case:", which is "right" I think, but 
aligning those with switch() seems to be pretty standard to conserve space.

> Cheers,
> Jes