Patchwork [02/18] qemu-sockets: add Error ** to all functions

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 3, 2012, 2:36 p.m.
Message ID <1349275025-5093-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/188789/
State New
Headers show

Comments

Paolo Bonzini - Oct. 3, 2012, 2:36 p.m.
This lets me adjust the clients to do proper error propagation first,
thus avoiding temporary regressions in the quality of the error messages.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c               |  4 ++--
 qemu-char.c         |  6 +++---
 qemu-sockets.c      | 30 +++++++++++++++---------------
 qemu_socket.h       | 10 +++++-----
 qga/channel-posix.c |  2 +-
 ui/vnc.c            |  4 ++--
 6 file modificati, 28 inserzioni(+), 28 rimozioni(-)
Luiz Capitulino - Oct. 4, 2012, 4:19 p.m.
On Wed,  3 Oct 2012 16:36:49 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This lets me adjust the clients to do proper error propagation first,
> thus avoiding temporary regressions in the quality of the error messages.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  nbd.c               |  4 ++--
>  qemu-char.c         |  6 +++---
>  qemu-sockets.c      | 30 +++++++++++++++---------------
>  qemu_socket.h       | 10 +++++-----
>  qga/channel-posix.c |  2 +-
>  ui/vnc.c            |  4 ++--
>  6 file modificati, 28 inserzioni(+), 28 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..3cc6cb5 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2102,7 +2102,7 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>      chr = g_malloc0(sizeof(CharDriverState));
>      s = g_malloc0(sizeof(NetCharDriver));
>  
> -    fd = inet_dgram_opts(opts);
> +    fd = inet_dgram_opts(opts, NULL);
>      if (fd < 0) {
>          fprintf(stderr, "inet_dgram_opts failed\n");
>          goto return_err;
> @@ -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 1f14e8b..5cf2b32 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -404,7 +404,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>      return sock;
>  }
>  
> -int inet_dgram_opts(QemuOpts *opts)
> +int inet_dgram_opts(QemuOpts *opts, Error **errp)
>  {
>      struct addrinfo ai, *peer = NULL, *local = NULL;
>      const char *addr;
> @@ -654,7 +654,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");
> @@ -702,7 +702,7 @@ 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");
> @@ -732,7 +732,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;
> @@ -753,7 +753,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 : "");
> @@ -761,44 +761,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");

As we've discussed in a previous series, this breaks error reporting
for unix_listen_opts() until errp is passed _and_ the error is routed
to the right direction (in this case stderr).

Is this fixed later in this series? If it's not, when do you plan to fix it?

>      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");
>      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");
>      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");
>      errno = ENOTSUP;
>      return -1;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index 3e8aee9..ff979b5 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -53,13 +53,13 @@ int inet_nonblocking_connect(const char *str,
>                               NonBlockingConnectHandler *callback,
>                               void *opaque, Error **errp);
>  
> -int inet_dgram_opts(QemuOpts *opts);
> +int inet_dgram_opts(QemuOpts *opts, Error **errp);
>  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. 4, 2012, 4:39 p.m.
Il 04/10/2012 18:19, Luiz Capitulino ha scritto:
>> >  
>> > -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");
> As we've discussed in a previous series, this breaks error reporting
> for unix_listen_opts() until errp is passed _and_ the error is routed
> to the right direction (in this case stderr).
> 
> Is this fixed later in this series? If it's not, when do you plan to fix it?

It is (which is why the single patch became 18), and I can move this
hunk at the end of the series.  Note that is just for Windows code and
should really never run.

Paolo
Luiz Capitulino - Oct. 4, 2012, 4:41 p.m.
On Thu, 04 Oct 2012 18:39:06 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/10/2012 18:19, Luiz Capitulino ha scritto:
> >> >  
> >> > -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");
> > As we've discussed in a previous series, this breaks error reporting
> > for unix_listen_opts() until errp is passed _and_ the error is routed
> > to the right direction (in this case stderr).
> > 
> > Is this fixed later in this series? If it's not, when do you plan to fix it?
> 
> It is (which is why the single patch became 18), and I can move this
> hunk at the end of the series.  Note that is just for Windows code and
> should really never run.

Right:

 Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Markus Armbruster - Oct. 17, 2012, 1:10 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/10/2012 18:19, Luiz Capitulino ha scritto:
>>> >  
>>> > -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");

Just gave me a "review WTF", too.

>> As we've discussed in a previous series, this breaks error reporting
>> for unix_listen_opts() until errp is passed _and_ the error is routed
>> to the right direction (in this case stderr).
>> 
>> Is this fixed later in this series? If it's not, when do you plan to fix it?
>
> It is (which is why the single patch became 18), and I can move this
> hunk at the end of the series.  Note that is just for Windows code and
> should really never run.

Fair enough, but a note in the commit message is in order.

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..3cc6cb5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2102,7 +2102,7 @@  static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
     chr = g_malloc0(sizeof(CharDriverState));
     s = g_malloc0(sizeof(NetCharDriver));
 
-    fd = inet_dgram_opts(opts);
+    fd = inet_dgram_opts(opts, NULL);
     if (fd < 0) {
         fprintf(stderr, "inet_dgram_opts failed\n");
         goto return_err;
@@ -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 1f14e8b..5cf2b32 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -404,7 +404,7 @@  int inet_connect_opts(QemuOpts *opts, Error **errp,
     return sock;
 }
 
-int inet_dgram_opts(QemuOpts *opts)
+int inet_dgram_opts(QemuOpts *opts, Error **errp)
 {
     struct addrinfo ai, *peer = NULL, *local = NULL;
     const char *addr;
@@ -654,7 +654,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");
@@ -702,7 +702,7 @@  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");
@@ -732,7 +732,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;
@@ -753,7 +753,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 : "");
@@ -761,44 +761,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");
     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");
     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");
     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");
     errno = ENOTSUP;
     return -1;
 }
diff --git a/qemu_socket.h b/qemu_socket.h
index 3e8aee9..ff979b5 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -53,13 +53,13 @@  int inet_nonblocking_connect(const char *str,
                              NonBlockingConnectHandler *callback,
                              void *opaque, Error **errp);
 
-int inet_dgram_opts(QemuOpts *opts);
+int inet_dgram_opts(QemuOpts *opts, Error **errp);
 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);