Re: [Qemu-devel] [PATCH v2 4/9] qemu-sockets: add error propagation to Unix socket functions

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon,  1 Oct 2012 16:52:19 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This patch starts harmonizing unix_* and inet_* functions.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  nbd.c               |  4 ++--
>  qemu-char.c         |  4 ++--
>  qemu-sockets.c      | 40 ++++++++++++++++++++--------------------
>  qemu_socket.h       |  8 ++++----
>  qga/channel-posix.c |  2 +-
>  ui/vnc.c            |  4 ++--
>  6 file modificati, 31 inserzioni(+), 31 rimozioni(-)
> 
> diff --git a/nbd.c b/nbd.c
> index 6f0db62..f61a288 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -230,12 +230,12 @@ int unix_socket_incoming(const char *path)
>      char *ostr = NULL;
>      int olen = 0;
>  
> -    return unix_listen(path, ostr, olen);
> +    return unix_listen(path, ostr, olen, NULL);
>  }
>  
>  int unix_socket_outgoing(const char *path)
>  {
> -    return unix_connect(path);
> +    return unix_connect(path, NULL);
>  }
>  
>  /* Basic flow for negotiation
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..fa294c0 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2448,9 +2448,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>  
>      if (is_unix) {
>          if (is_listen) {
> -            fd = unix_listen_opts(opts);
> +            fd = unix_listen_opts(opts, NULL);
>          } else {
> -            fd = unix_connect_opts(opts);
> +            fd = unix_connect_opts(opts, NULL);
>          }
>      } else {
>          if (is_listen) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index bf1f794..1e69274 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -655,7 +655,7 @@ int inet_nonblocking_connect(const char *str,
>  
>  #ifndef _WIN32
>  
> -int unix_listen_opts(QemuOpts *opts)
> +int unix_listen_opts(QemuOpts *opts, Error **errp)
>  {
>      struct sockaddr_un un;
>      const char *path = qemu_opt_get(opts, "path");
> @@ -663,7 +663,7 @@ int unix_listen_opts(QemuOpts *opts)
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> -        perror("socket(unix)");
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>  
> @@ -688,11 +688,11 @@ 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));
> +        error_set(errp, QERR_SOCKET_BIND_FAILED);

This drops error information, making the error message worse. I believe
you have a reason to not use error_setg()?

Also, I see that in some hunks you do something like:

  -            fd = unix_listen_opts(opts);
  +            fd = unix_listen_opts(opts, NULL);

This will break printing the error message to the user. It's fine by me if
you do this only temporarily (ie. this is fixed by the next or a later patch),
but want to double check that you're aware of it.

Btw, I'm making these comments in this hunk but they apply to similar hunks
as well.

>          goto err;
>      }
>      if (listen(sock, 1) < 0) {
> -        fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno));
> +        error_set(errp, QERR_SOCKET_LISTEN_FAILED);
>          goto err;
>      }
>  
> @@ -703,20 +703,20 @@ err:
>      return -1;
>  }
>  
> -int unix_connect_opts(QemuOpts *opts)
> +int unix_connect_opts(QemuOpts *opts, Error **errp)
>  {
>      struct sockaddr_un un;
>      const char *path = qemu_opt_get(opts, "path");
>      int sock;
>  
>      if (NULL == path) {
> -        fprintf(stderr, "unix connect: no path specified\n");
> +        error_setg(errp, "unix connect: no path specified\n");
>          return -1;
>      }
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> -        perror("socket(unix)");
> +        error_set(errp, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>  
> @@ -724,7 +724,7 @@ int unix_connect_opts(QemuOpts *opts)
>      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));
> +        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
>          close(sock);
>  	return -1;
>      }
> @@ -733,7 +733,7 @@ int unix_connect_opts(QemuOpts *opts)
>  }
>  
>  /* compatibility wrapper */
> -int unix_listen(const char *str, char *ostr, int olen)
> +int unix_listen(const char *str, char *ostr, int olen, Error **errp)
>  {
>      QemuOpts *opts;
>      char *path, *optstr;
> @@ -754,7 +754,7 @@ int unix_listen(const char *str, char *ostr, int olen)
>          qemu_opt_set(opts, "path", str);
>      }
>  
> -    sock = unix_listen_opts(opts);
> +    sock = unix_listen_opts(opts, errp);
>  
>      if (sock != -1 && ostr)
>          snprintf(ostr, olen, "%s%s", qemu_opt_get(opts, "path"), optstr ? optstr : "");
> @@ -762,44 +762,44 @@ int unix_listen(const char *str, char *ostr, int olen)
>      return sock;
>  }
>  
> -int unix_connect(const char *path)
> +int unix_connect(const char *path, Error **errp)
>  {
>      QemuOpts *opts;
>      int sock;
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      qemu_opt_set(opts, "path", path);
> -    sock = unix_connect_opts(opts);
> +    sock = unix_connect_opts(opts, errp);
>      qemu_opts_del(opts);
>      return sock;
>  }
>  
>  #else
>  
> -int unix_listen_opts(QemuOpts *opts)
> +int unix_listen_opts(QemuOpts *opts, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
>  
> -int unix_connect_opts(QemuOpts *opts)
> +int unix_connect_opts(QemuOpts *opts, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
>  
> -int unix_listen(const char *path, char *ostr, int olen)
> +int unix_listen(const char *path, char *ostr, int olen, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
>  
> -int unix_connect(const char *path)
> +int unix_connect(const char *path, Error **errp)
>  {
> -    fprintf(stderr, "unix sockets are not available on windows\n");
> +    error_setg(errp, "unix sockets are not available on windows\n");
>      errno = ENOTSUP;
>      return -1;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 3e8aee9..afe8689 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -56,10 +56,10 @@ int inet_nonblocking_connect(const char *str,
>  int inet_dgram_opts(QemuOpts *opts);
>  const char *inet_strfamily(int family);
>  
> -int unix_listen_opts(QemuOpts *opts);
> -int unix_listen(const char *path, char *ostr, int olen);
> -int unix_connect_opts(QemuOpts *opts);
> -int unix_connect(const char *path);
> +int unix_listen_opts(QemuOpts *opts, Error **errp);
> +int unix_listen(const char *path, char *ostr, int olen, Error **errp);
> +int unix_connect_opts(QemuOpts *opts, Error **errp);
> +int unix_connect(const char *path, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index 57eea06..e22eee6 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -181,7 +181,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
>          break;
>      }
>      case GA_CHANNEL_UNIX_LISTEN: {
> -        int fd = unix_listen(path, NULL, strlen(path));
> +        int fd = unix_listen(path, NULL, strlen(path), NULL);
>          if (fd == -1) {
>              g_critical("error opening path: %s", strerror(errno));
>              return false;
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 01b2daf..235596e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3059,7 +3059,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>      if (reverse) {
>          /* connect to viewer */
>          if (strncmp(display, "unix:", 5) == 0)
> -            vs->lsock = unix_connect(display+5);
> +            vs->lsock = unix_connect(display+5, NULL);
>          else
>              vs->lsock = inet_connect(display, NULL);
>          if (-1 == vs->lsock) {
> @@ -3079,7 +3079,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>          dpy = g_malloc(256);
>          if (strncmp(display, "unix:", 5) == 0) {
>              pstrcpy(dpy, 256, "unix:");
> -            vs->lsock = unix_listen(display+5, dpy+5, 256-5);
> +            vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
>          } else {
>              vs->lsock = inet_listen(display, dpy, 256,
>                                      SOCK_STREAM, 5900, NULL);