Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part

[Anthony Liguori <aliguori@us.ibm.com>]

On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> Convert to error_report().  This adds error locations to the messages,
> which is particularly important when the location is buried in a
> configuration file.  Moreover, we'll need this when we create a
> monitor command to add character devices, so its errors actually
> appear in the monitor, not stderr.
>
> Also clean up the messages, and get rid of some that look like errors,
> but aren't.
>
> Improves user-hostile messages like this one for "-chardev
> socket,id=foo,host=blackfin,port=1,server"
>
>      inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied
>      chardev: opening backend "socket" failed
>
> to
>
>      qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server: Can't bind port blackfin:1: Permission denied
>      chardev: opening backend "socket" failed
>
> and this one for "-chardev udp,id=foo,localport=1,port=1"
>
>      inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK
>      inet_dgram_opts failed
>      chardev: opening backend "udp" failed
>
> to
>
>      qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind port :1: Permission denied
>      chardev: opening backend "udp" failed
>
> You got to love the "OK" part.
>
> The uninformative extra "opening backend failed" message will be
> cleaned up shortly.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   qemu-char.c    |    1 -
>   qemu-sockets.c |  154 +++++++++++++++++++++++++------------------------------
>   2 files changed, 70 insertions(+), 85 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index bb9e3f5..d591f70 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>
>       fd = inet_dgram_opts(opts);
>       if (fd<  0) {
> -        fprintf(stderr, "inet_dgram_opts failed\n");
>           goto return_err;
>       }

Let's add an Error ** argument here.

It's easy to bridge errors to error_report (qerror_report_err) so it's conducive 
to incremental refactoring.  Plus it starts getting us away from terminal errors 
and into proper erroro propagation.

> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 6bcb8e3..67e0559 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -20,7 +20,8 @@
>   #include<unistd.h>
>
>   #include "qemu_socket.h"
> -#include "qemu-common.h" /* for qemu_isdigit */
> +#include "qemu-common.h"
> +#include "qemu-error.h"
>
>   #ifndef AI_ADDRCONFIG
>   # define AI_ADDRCONFIG 0
> @@ -107,7 +108,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
>       char port[33];
>       char uaddr[INET6_ADDRSTRLEN+1];
>       char uport[33];
> -    int slisten, rc, to, port_min, port_max, p;
> +    int slisten, rc, to, port_min, port_max, p, sav_errno;
>
>       memset(&ai,0, sizeof(ai));
>       ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
> @@ -116,7 +117,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
>
>       if ((qemu_opt_get(opts, "host") == NULL) ||
>           (qemu_opt_get(opts, "port") == NULL)) {
> -        fprintf(stderr, "%s: host and/or port not specified\n", __FUNCTION__);
> +        error_report("inet socket character device requires parameters host and port");
>           return -1;
>       }
>       pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port"));
> @@ -133,8 +134,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
>           snprintf(port, sizeof(port), "%d", atoi(port) + port_offset);
>       rc = getaddrinfo(strlen(addr) ? addr : NULL, port,&ai,&res);
>       if (rc != 0) {
> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> +        error_report("Can't resolve %s:%s: %s",
> +                     addr, port, gai_strerror(rc));
>           return -1;
>       }
>
> @@ -145,9 +146,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
>   		        NI_NUMERICHOST | NI_NUMERICSERV);
>           slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
>           if (slisten<  0) {
> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
> -                    inet_strfamily(e->ai_family), strerror(errno));
> -            continue;
> +            continue;           /* try next address */
>           }
>
>           setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
> @@ -166,25 +165,33 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
>               if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
>                   goto listen;
>               }
> -            if (p == port_max) {
> -                fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__,
> -                        inet_strfamily(e->ai_family), uaddr, inet_getport(e),
> -                        strerror(errno));
> -            }
>           }
> +
> +        sav_errno = errno;
>           closesocket(slisten);
> +        errno = sav_errno;
> +        /* try next address */
> +    }
> +
> +    /* no address worked, errno is from last failed socket() or bind() */
> +    if (to) {
> +        error_report("Can't bind any port %s:%s..%d: %s",
> +                     addr, port, to, strerror(errno));
> +    } else {
> +        error_report("Can't bind port %s:%s: %s",
> +                     addr, port, strerror(errno));
>       }
> -    fprintf(stderr, "%s: FAILED\n", __FUNCTION__);
>       freeaddrinfo(res);
>       return -1;
>
>   listen:
>       if (listen(slisten,1) != 0) {
> -        perror("listen");
> +        error_report("Can't listen on %s:%d: %s", addr, p, strerror(errno));
>           closesocket(slisten);
>           freeaddrinfo(res);
>           return -1;
>       }
> +
>       snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset);
>       qemu_opt_set(opts, "host", uaddr);
>       qemu_opt_set(opts, "port", uport);
> @@ -199,8 +206,6 @@ int inet_connect_opts(QemuOpts *opts)
>       struct addrinfo ai,*res,*e;
>       const char *addr;
>       const char *port;
> -    char uaddr[INET6_ADDRSTRLEN+1];
> -    char uport[33];
>       int sock,rc;
>
>       memset(&ai,0, sizeof(ai));
> @@ -211,7 +216,7 @@ int inet_connect_opts(QemuOpts *opts)
>       addr = qemu_opt_get(opts, "host");
>       port = qemu_opt_get(opts, "port");
>       if (addr == NULL || port == NULL) {
> -        fprintf(stderr, "inet_connect: host and/or port not specified\n");
> +        error_report("inet socket character device requires parameters host and port");
>           return -1;
>       }
>
> @@ -222,38 +227,29 @@ int inet_connect_opts(QemuOpts *opts)
>
>       /* lookup */
>       if (0 != (rc = getaddrinfo(addr, port,&ai,&res))) {
> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> +        error_report("Can't resolve %s:%s: %s",
> +                     addr, port, gai_strerror(rc));
>   	return -1;
>       }
>
>       for (e = res; e != NULL; e = e->ai_next) {
> -        if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
> -                            uaddr,INET6_ADDRSTRLEN,uport,32,
> -                            NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
> -            fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__);
> -            continue;
> -        }
>           sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
>           if (sock<  0) {
> -            fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
> -            inet_strfamily(e->ai_family), strerror(errno));
> -            continue;
> +            continue;           /* try next address */
>           }
>           setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>
>           /* connect to peer */
>           if (connect(sock,e->ai_addr,e->ai_addrlen)<  0) {
> -            if (NULL == e->ai_next)
> -                fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> -                        inet_strfamily(e->ai_family),
> -                        e->ai_canonname, uaddr, uport, strerror(errno));
>               closesocket(sock);
> -            continue;
> +            continue;           /* try next address */
>           }
>           freeaddrinfo(res);
>           return sock;
>       }
> +
> +    /* no address worked, errno is from last failed socket() or connect() */
> +    error_report("Can't connect to %s:%s: %s", addr, port, strerror(errno));
>       freeaddrinfo(res);
>       return -1;
>   }
> @@ -261,12 +257,9 @@ int inet_connect_opts(QemuOpts *opts)
>   int inet_dgram_opts(QemuOpts *opts)
>   {
>       struct addrinfo ai, *peer = NULL, *local = NULL;
> -    const char *addr;
> -    const char *port;
> -    char uaddr[INET6_ADDRSTRLEN+1];
> -    char uport[33];
> +    const char *addr, *port;
> +    const char *localaddr, *localport;
>       int sock = -1, rc;
> -
>       /* lookup peer addr */
>       memset(&ai,0, sizeof(ai));
>       ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
> @@ -279,7 +272,7 @@ int inet_dgram_opts(QemuOpts *opts)
>           addr = "localhost";
>       }
>       if (port == NULL || strlen(port) == 0) {
> -        fprintf(stderr, "inet_dgram: port not specified\n");
> +        error_report("udp character device requires parameter port");
>           return -1;
>       }
>
> @@ -289,8 +282,8 @@ int inet_dgram_opts(QemuOpts *opts)
>           ai.ai_family = PF_INET6;
>
>       if (0 != (rc = getaddrinfo(addr, port,&ai,&peer))) {
> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> +        error_report("Can't resolve %s:%s: %s",
> +                     addr, port, gai_strerror(rc));
>   	return -1;
>       }
>
> @@ -300,53 +293,40 @@ int inet_dgram_opts(QemuOpts *opts)
>       ai.ai_family = peer->ai_family;
>       ai.ai_socktype = SOCK_DGRAM;
>
> -    addr = qemu_opt_get(opts, "localaddr");
> -    port = qemu_opt_get(opts, "localport");
> -    if (addr == NULL || strlen(addr) == 0) {
> -        addr = NULL;
> +    localaddr = qemu_opt_get(opts, "localaddr");
> +    localport = qemu_opt_get(opts, "localport");
> +    if (localaddr == NULL || strlen(localaddr) == 0) {
> +        localaddr = NULL;
>       }
> -    if (!port || strlen(port) == 0)
> -        port = "0";
> +    if (!localport || strlen(localport) == 0)
> +        localport = "0";
>
> -    if (0 != (rc = getaddrinfo(addr, port,&ai,&local))) {
> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> +    if (0 != (rc = getaddrinfo(localaddr, localport,&ai,&local))) {
> +        error_report("Can't resolve %s:%s: %s",
> +                     localaddr ? localaddr : NULL, localport,
> +                     gai_strerror(rc));
>           return -1;
>       }
>
>       /* create socket */
>       sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol);
>       if (sock<  0) {
> -        fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__,
> -                inet_strfamily(peer->ai_family), strerror(errno));
> -        goto err;
> +        goto cant_bind;
>       }
>       setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on));
>
>       /* bind socket */
> -    if (getnameinfo((struct sockaddr*)local->ai_addr,local->ai_addrlen,
> -                    uaddr,INET6_ADDRSTRLEN,uport,32,
> -                    NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
> -        fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__);
> -        goto err;
> -    }
>       if (bind(sock, local->ai_addr, local->ai_addrlen)<  0) {
> -        fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
> -                inet_strfamily(local->ai_family), uaddr, inet_getport(local));
> +    cant_bind:
> +        error_report("Can't bind port %s:%s: %s",
> +                     localaddr ? localaddr : "", localport, strerror(errno));
>           goto err;
>       }
>
>       /* connect to peer */
> -    if (getnameinfo((struct sockaddr*)peer->ai_addr, peer->ai_addrlen,
> -                    uaddr, INET6_ADDRSTRLEN, uport, 32,
> -                    NI_NUMERICHOST | NI_NUMERICSERV) != 0) {
> -        fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__);
> -        goto err;
> -    }
>       if (connect(sock,peer->ai_addr,peer->ai_addrlen)<  0) {
> -        fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__,
> -                inet_strfamily(peer->ai_family),
> -                peer->ai_canonname, uaddr, uport, strerror(errno));
> +        error_report("Can't connect to %s:%s: %s",
> +                     addr, port, strerror(errno));
>           goto err;
>       }
>
> @@ -471,8 +451,7 @@ int unix_listen_opts(QemuOpts *opts)
>
>       sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>       if (sock<  0) {
> -        perror("socket(unix)");
> -        return -1;
> +        goto err;
>       }
>
>       memset(&un, 0, sizeof(un));
> @@ -496,18 +475,20 @@ int unix_listen_opts(QemuOpts *opts)
>
>       unlink(un.sun_path);
>       if (bind(sock, (struct sockaddr*)&un, sizeof(un))<  0) {
> -        fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno));
>           goto err;
>       }
>       if (listen(sock, 1)<  0) {
> -        fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno));
>           goto err;
>       }
>
>       return sock;
>
>   err:
> -    closesocket(sock);
> +    error_report("Can't create socket %s: %s",
> +                 un.sun_path, strerror(errno));
> +    if (sock>= 0) {
> +        closesocket(sock);
> +    }
>       return -1;
>   }
>
> @@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts)
>       int sock;
>
>       if (NULL == path) {
> -        fprintf(stderr, "unix connect: no path specified\n");
> +        error_report("unix socket character device requires parameter path");
>           return -1;
>       }
>
>       sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>       if (sock<  0) {
> -        perror("socket(unix)");
> -        return -1;
> +        goto err;
>       }
>
>       memset(&un, 0, sizeof(un));
>       un.sun_family = AF_UNIX;
>       snprintf(un.sun_path, sizeof(un.sun_path), "%s", path);
>       if (connect(sock, (struct sockaddr*)&un, sizeof(un))<  0) {
> -        fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
> -        close(sock);
> -	return -1;
> +        goto err;
>       }
>
>       return sock;
> +
> +err:
> +    error_report("Can't connect to socket %s: %s",
> +                 un.sun_path, strerror(errno));
> +    if (sock>= 0) {
> +        close(sock);
> +    }
> +    return -1;
>   }

Basically, same thing here and the remaining functions.  Let's not introduce 
additional uses of error_report().

That said, I imagine you don't want to introduce a bunch of error types for 
these different things and that's probably not productive anyway.

So let's compromise and introduce a generic QERR_INTERNAL_ERROR that takes a 
single human readable string as an argument.  We can have a wrapper for it that 
also records location information in the error object.

Regards,

Anthony Liguori