Re: [Qemu-devel] [PATCH v7 4/4] net/socket: Improve -net socket error reporting

[Mao Zhongyi]

On 07/05/2017 12:24 AM, Markus Armbruster wrote:
> Mao Zhongyi <address@hidden> writes:
>
> > When -net socket fails, it first reports a specific error, then
> > a generic one, like this:
> >
> >     $ qemu-system-x86_64 -net socket,
> >     qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, 
> > mcast= or udp= is required
> >     qemu-system-x86_64: -net socket: Device 'socket' could not be initialized
> >
> > Convert net_socket_*_init() to Error to get rid of the superfluous second
> > error message. After the patch, the effect like this:
> >
> >     $ qemu-system-x86_64 -net socket,
> >     qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, 
> > mcast= or udp= is required
> >
> > At the same time, add many explicit error handling message when it fails.
> >
> > Cc: address@hidden
> > Cc: address@hidden
> > Cc: address@hidden
> > Signed-off-by: Mao Zhongyi <address@hidden>
> > ---
> >  net/socket.c | 94 +++++++++++++++++++++++++++++-------------------------------
> >  1 file changed, 45 insertions(+), 49 deletions(-)
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index bd80b3c..a891c3a 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -494,22 +494,21 @@ static void net_socket_accept(void *opaque)
> >  static int net_socket_listen_init(NetClientState *peer,
> >                                    const char *model,
> >                                    const char *name,
> > -                                  const char *host_str)
> > +                                  const char *host_str,
> > +                                  Error **errp)
> >  {
> >      NetClientState *nc;
> >      NetSocketState *s;
> >      struct sockaddr_in saddr;
> >      int fd, ret;
> > -    Error *err = NULL;
> >
> > -    if (parse_host_port(&saddr, host_str, &err) < 0) {
> > -        error_report_err(err);
> > +    if (parse_host_port(&saddr, host_str, errp) < 0) {
> >          return -1;
> >      }
> >
> >      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> >      if (fd < 0) {
> > -        perror("socket");
> > +        error_setg_errno(errp, errno, "failed to create stream socket");
> >          return -1;
> >      }
> >      qemu_set_nonblock(fd);
> > @@ -518,13 +517,14 @@ static int net_socket_listen_init(NetClientState *peer,
> >
> >      ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
> >      if (ret < 0) {
> > -        perror("bind");
> > +        error_setg_errno(errp, errno, "bind ip=%s to socket failed",
> > +                         inet_ntoa(saddr.sin_addr));
>
> Comment on the same error message in PATCH 2 applies.

OK, I will fix the similar problems right away.

> >          closesocket(fd);
> >          return -1;
> >      }
> >      ret = listen(fd, 0);
> >      if (ret < 0) {
> > -        perror("listen");
> > +        error_setg_errno(errp, errno, "listen socket failed");
>
> Suggest "can't listen on socket".

OK, I will.


> > -    fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
[...]
> >      if (sock->has_listen) {
> > -        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
> > +        if (net_socket_listen_init(peer, "socket", name,
> > +            sock->listen, errp) == -1) {
>
> Please avoid breaking the line in the middle of an operator's operand
> when you could just as well break it at the operator, like this:
>
>            if (net_socket_listen_init(peer, "socket", name, sock->listen, errp)
>                == -1) {
>
> Since you're touching this line anyway, I suggest to replace == -1 by
> the more idiomatic < 0.

OK, I see.

Thanks,
Mao