Patchwork [v2,4/9] qemu-sockets: add error propagation to Unix socket functions

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 1, 2012, 2:52 p.m.
Message ID <1349103144-6827-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/188316/
State New
Headers show

Comments

Paolo Bonzini - Oct. 1, 2012, 2:52 p.m.
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(-)
Luiz Capitulino - Oct. 1, 2012, 5:17 p.m.
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);
Paolo Bonzini - Oct. 1, 2012, 7:07 p.m.
Il 01/10/2012 19:17, Luiz Capitulino ha scritto:
>> >      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()?

I was waiting for the end of the discussion on errno to add
error_setg_errno.

> 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.

I want to avoid super-large patch series, so I would prefer to fix it
later in the 1.3 development.

Paolo

> Btw, I'm making these comments in this hunk but they apply to similar hunks
> as well.
>
Luiz Capitulino - Oct. 1, 2012, 11:05 p.m.
On Mon, 01 Oct 2012 21:07:52 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 01/10/2012 19:17, Luiz Capitulino ha scritto:
> >> >      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()?
> 
> I was waiting for the end of the discussion on errno to add
> error_setg_errno.

The decision was to not add errno now, right?

> > 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.
> 
> I want to avoid super-large patch series, so I would prefer to fix it
> later in the 1.3 development.

We at least need to have the patches flying, I don't think it's ok to
break error reporting like that.
Paolo Bonzini - Oct. 2, 2012, 6:09 a.m.
Il 02/10/2012 01:05, Luiz Capitulino ha scritto:
> On Mon, 01 Oct 2012 21:07:52 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 01/10/2012 19:17, Luiz Capitulino ha scritto:
>>>>>      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()?
>>
>> I was waiting for the end of the discussion on errno to add
>> error_setg_errno.
> 
> The decision was to not add errno now, right?

I don't remember, but we can still add a function that takes an errno
value and tacks on the strerror(errno).

>>> 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.
>>
>> I want to avoid super-large patch series, so I would prefer to fix it
>> later in the 1.3 development.
> 
> We at least need to have the patches flying, I don't think it's ok to
> break error reporting like that.

I disagree because we still have two months before release, but I'll see
what I can do.

Paolo

Patch

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