[Qemu-devel] [PATCH 00/18] qemu-sockets error propagation and related cleanups

[Qemu-devel] [PATCH 00/18] qemu-sockets error propagation and related cleanups

[Paolo Bonzini]

Luiz,

here are the patches to improve the quality of qemu-sockets Error objects
including the errno.  In some cases there is a small regression in that
the numeric address disappears, but in general the benefit very much
overwhelms the disadvantages.  For example:

    $ qemu-system-x86_64 -monitor tcp:foo.bar:12345
    getaddrinfo(foo.bar,12345): Name or service not known
    chardev: opening backend "socket" failed

    $ qemu-system-x86_64 -monitor tcp:localhost:443,server=on
    inet_listen_opts: bind(ipv4,127.0.0.1,443): Permission denied
    inet_listen_opts: FAILED
    chardev: opening backend "socket" failed

becomes:

    $ qemu-system-x86_64 -monitor tcp:foo.bar:12345
    qemu-system-x86_64: -monitor tcp:foo.bar:12345: address resolution failed for foo.bar:12345: Name or service not known
    chardev: opening backend "socket" failed

    $ qemu-system-x86_64 -monitor tcp:localhost:443,server=on
    qemu-system-x86_64: -monitor tcp:localhost:443,server=on: Failed to bind socket: Permission denied
    chardev: opening backend "socket" failed

It is possible to add back the family/host/port subsequently, and also
to perfect TCP migration errors.  Those patches should be small and
acceptable closer to release.

In addition, the patches remove some duplication in the handling of
Unix sockets, with the end result that the patches remove more code
than they add!

Patches 1 and 2 are small preparations.  Two new functions are added,
error_set_errno and error_setg_errno, which take care of adding a
strerror() result to error messages.  Then, the new prototypes are
introduced for qemu-sockets functions.

Patches 3 and 4 port Orit's NonBlockingConnectHandler work to Unix
sockets, which is used later for Unix socket migration.

Patches 5 to 9 add proper error propagation to migration and remove
migration-specific Unix socket code.  Patches 10 to 13 teach the other
qemu-sockets users to consume Error objects.

Patches 14 to 18 then teach Error production and propagation to
qemu-sockets.

Paolo Bonzini (18):
  error: add error_set_errno and error_setg_errno
  qemu-sockets: add Error ** to all functions
  qemu-sockets: unix_listen and unix_connect are portable
  qemu-sockets: add nonblocking connect for Unix sockets
  migration: avoid using error_is_set
  migration: centralize call to migrate_fd_error()
  migration: use qemu-sockets to establish Unix sockets
  migration (outgoing): add error propagation for fd and exec protocols
  migration (incoming): add error propagation for fd and exec protocols
  qemu-char: ask and print error information from qemu-sockets
  nbd: ask and print error information from qemu-sockets
  qemu-ga: ask and print error information from qemu-sockets
  vnc: add error propagation to vnc_display_open
  qemu-sockets: include strerror or gai_strerror output in error
    messages
  qemu-sockets: add error propagation to inet_connect_addr
  qemu-sockets: add error propagation to inet_dgram_opts
  qemu-sockets: add error propagation to inet_parse
  qemu-sockets: add error propagation to Unix socket functions

 console.h           |   2 +-
 error.c             |  28 ++++++
 error.h             |   9 ++
 migration-exec.c    |  16 ++--
 migration-fd.c      |  19 ++--
 migration-tcp.c     |  19 +---
 migration-unix.c    |  95 +++-----------------
 migration.c         |  34 +++----
 migration.h         |  19 ++--
 nbd.c               |  39 ++++++--
 qemu-char.c         |  24 +++--
 qemu-sockets.c      | 253 +++++++++++++++++++++++++++-------------------------
 qemu_socket.h       |  14 +--
 qga/channel-posix.c |   8 +-
 qmp.c               |   6 +-
 ui/vnc.c            |  67 +++++++-------
 vl.c                |  21 +++--
 17 file modificati, 326 inserzioni(+), 347 rimozioni(-)

-- 
1.7.12.1

[Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Paolo Bonzini]

These functions help maintaining homogeneous formatting of error
messages that include strerror values.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 error.c | 28 ++++++++++++++++++++++++++++
 error.h |  9 +++++++++
 2 file modificati, 37 inserzioni(+)

diff --git a/error.c b/error.c
index 1f05fc4..128d88c 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     *errp = err;
 }
 
+void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
+                     const char *fmt, ...)
+{
+    Error *err;
+    char *msg1;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+    assert(*errp == NULL);
+
+    err = g_malloc0(sizeof(*err));
+
+    va_start(ap, fmt);
+    msg1 = g_strdup_vprintf(fmt, ap);
+    if (os_errno != 0) {
+        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
+        g_free(msg1);
+    } else {
+        err->msg = msg1;
+    }
+    va_end(ap);
+    err->err_class = err_class;
+
+    *errp = err;
+}
+
 Error *error_copy(const Error *err)
 {
     Error *err_new;
diff --git a/error.h b/error.h
index da7fed3..4d52e73 100644
--- a/error.h
+++ b/error.h
@@ -30,10 +30,19 @@ typedef struct Error Error;
 void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 
 /**
+ * Set an indirect pointer to an error given a ErrorClass value and a
+ * printf-style human message, followed by a strerror() string if
+ * @os_error is not zero.
+ */
+void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+
+/**
  * Same as error_set(), but sets a generic error
  */
 #define error_setg(err, fmt, ...) \
     error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+#define error_setg_errno(err, os_error, fmt, ...) \
+    error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 
 /**
  * Returns true if an indirect pointer to an error is pointing to a valid
-- 
1.7.12.1

[Qemu-devel] [PATCH 02/18] qemu-sockets: add Error ** to all functions

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 03/18] qemu-sockets: unix_listen and unix_connect are portable

[Paolo Bonzini]

They are just wrappers and do not need a Win32-specific version.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 49 +++++++++++++++++--------------------------------
 1 file modificato, 17 inserzioni(+), 32 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 5cf2b32..79c7b66 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -731,6 +731,23 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
     return sock;
 }
 
+#else
+
+int unix_listen_opts(QemuOpts *opts, Error **errp)
+{
+    error_setg(errp, "unix sockets are not available on windows");
+    errno = ENOTSUP;
+    return -1;
+}
+
+int unix_connect_opts(QemuOpts *opts, Error **errp)
+{
+    error_setg(errp, "unix sockets are not available on windows");
+    errno = ENOTSUP;
+    return -1;
+}
+#endif
+
 /* compatibility wrapper */
 int unix_listen(const char *str, char *ostr, int olen, Error **errp)
 {
@@ -773,38 +790,6 @@ int unix_connect(const char *path, Error **errp)
     return sock;
 }
 
-#else
-
-int unix_listen_opts(QemuOpts *opts, Error **errp)
-{
-    error_setg(errp, "unix sockets are not available on windows");
-    errno = ENOTSUP;
-    return -1;
-}
-
-int unix_connect_opts(QemuOpts *opts, Error **errp)
-{
-    error_setg(errp, "unix sockets are not available on windows");
-    errno = ENOTSUP;
-    return -1;
-}
-
-int unix_listen(const char *path, char *ostr, int olen, Error **errp)
-{
-    error_setg(errp, "unix sockets are not available on windows");
-    errno = ENOTSUP;
-    return -1;
-}
-
-int unix_connect(const char *path, Error **errp)
-{
-    error_setg(errp, "unix sockets are not available on windows");
-    errno = ENOTSUP;
-    return -1;
-}
-
-#endif
-
 #ifdef _WIN32
 static void socket_cleanup(void)
 {
-- 
1.7.12.1

[Qemu-devel] [PATCH 04/18] qemu-sockets: add nonblocking connect for Unix sockets

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c    |  2 +-
 qemu-sockets.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 qemu_socket.h  |  6 ++++-
 3 file modificati, 72 inserzioni(+), 14 rimozioni(-)

diff --git a/qemu-char.c b/qemu-char.c
index 3cc6cb5..8ebd582 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2450,7 +2450,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
         if (is_listen) {
             fd = unix_listen_opts(opts, NULL);
         } else {
-            fd = unix_connect_opts(opts, NULL);
+            fd = unix_connect_opts(opts, NULL, NULL, NULL);
         }
     } else {
         if (is_listen) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 79c7b66..341ae21 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -252,16 +252,19 @@ static void wait_for_connect(void *opaque)
     }
 
     /* try to connect to the next address on the list */
-    while (s->current_addr->ai_next != NULL && s->fd < 0) {
-        s->current_addr = s->current_addr->ai_next;
-        s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
-        /* connect in progress */
-        if (in_progress) {
-            return;
+    if (s->current_addr) {
+        while (s->current_addr->ai_next != NULL && s->fd < 0) {
+            s->current_addr = s->current_addr->ai_next;
+            s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
+            /* connect in progress */
+            if (in_progress) {
+                return;
+            }
         }
+
+        freeaddrinfo(s->addr_list);
     }
 
-    freeaddrinfo(s->addr_list);
     if (s->callback) {
         s->callback(s->fd, s->opaque);
     }
@@ -702,32 +705,65 @@ err:
     return -1;
 }
 
-int unix_connect_opts(QemuOpts *opts, Error **errp)
+int unix_connect_opts(QemuOpts *opts, Error **errp,
+                      NonBlockingConnectHandler *callback, void *opaque)
 {
     struct sockaddr_un un;
     const char *path = qemu_opt_get(opts, "path");
-    int sock;
+    ConnectState *connect_state = NULL;
+    int sock, rc;
 
     if (NULL == path) {
         fprintf(stderr, "unix connect: no path specified\n");
         return -1;
     }
 
+    if (callback != NULL) {
+        connect_state = g_malloc0(sizeof(*connect_state));
+        connect_state->callback = callback;
+        connect_state->opaque = opaque;
+    }
+
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
         perror("socket(unix)");
         return -1;
     }
+    if (connect_state != NULL) {
+        socket_set_nonblock(sock);
+    }
 
     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) {
+
+    /* connect to peer */
+    do {
+        rc = 0;
+        if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
+            rc = -socket_error();
+        }
+    } while (rc == -EINTR);
+
+    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+        connect_state->fd = sock;
+        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
+                             connect_state);
+        return sock;
+    } else {
+        /* non blocking socket immediate success, call callback */
+        if (callback != NULL) {
+            callback(sock, opaque);
+        }
+    }
+
+    if (rc < 0) {
         fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
         close(sock);
 	return -1;
     }
 
+    g_free(connect_state);
     return sock;
 }
 
@@ -740,7 +776,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
     return -1;
 }
 
-int unix_connect_opts(QemuOpts *opts, Error **errp)
+int unix_connect_opts(QemuOpts *opts, Error **errp,
+                      NonBlockingConnectHandler *callback, void *opaque)
 {
     error_setg(errp, "unix sockets are not available on windows");
     errno = ENOTSUP;
@@ -785,7 +822,24 @@ int unix_connect(const char *path, Error **errp)
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
     qemu_opt_set(opts, "path", path);
-    sock = unix_connect_opts(opts, errp);
+    sock = unix_connect_opts(opts, errp, NULL, NULL);
+    qemu_opts_del(opts);
+    return sock;
+}
+
+
+int unix_nonblocking_connect(const char *path,
+                             NonBlockingConnectHandler *callback,
+                             void *opaque, Error **errp)
+{
+    QemuOpts *opts;
+    int sock = -1;
+
+    g_assert(callback != NULL);
+
+    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+    qemu_opt_set(opts, "path", path);
+    sock = unix_connect_opts(opts, errp, callback, opaque);
     qemu_opts_del(opts);
     return sock;
 }
diff --git a/qemu_socket.h b/qemu_socket.h
index ff979b5..89a5feb 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -58,8 +58,12 @@ const char *inet_strfamily(int family);
 
 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_opts(QemuOpts *opts, Error **errp,
+                      NonBlockingConnectHandler *callback, void *opaque);
 int unix_connect(const char *path, Error **errp);
+int unix_nonblocking_connect(const char *str,
+                             NonBlockingConnectHandler *callback,
+                             void *opaque, Error **errp);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
 int parse_host_port(struct sockaddr_in *saddr, const char *str);
-- 
1.7.12.1

[Qemu-devel] [PATCH 05/18] migration: avoid using error_is_set

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration-tcp.c |  8 +++++---
 migration.c     | 13 +++++++------
 2 file modificati, 12 inserzioni(+), 9 rimozioni(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index a15c2b8..78337a3 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -71,14 +71,16 @@ static void tcp_wait_for_connect(int fd, void *opaque)
 int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
                                  Error **errp)
 {
+    Error *local_err = NULL;
+
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
-                                     errp);
-    if (error_is_set(errp)) {
+    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
+    if (local_err != NULL) {
         migrate_fd_error(s);
+        error_propagate(errp, local_err);
         return -1;
     }
 
diff --git a/migration.c b/migration.c
index 22a05c4..8a04174 100644
--- a/migration.c
+++ b/migration.c
@@ -481,6 +481,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
 {
+    Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     MigrationParams params;
     const char *p;
@@ -506,7 +507,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     s = migrate_init(&params);
 
     if (strstart(uri, "tcp:", &p)) {
-        ret = tcp_start_outgoing_migration(s, p, errp);
+        ret = tcp_start_outgoing_migration(s, p, &local_err);
 #if !defined(WIN32)
     } else if (strstart(uri, "exec:", &p)) {
         ret = exec_start_outgoing_migration(s, p);
@@ -520,11 +521,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         return;
     }
 
-    if (ret < 0) {
-        if (!error_is_set(errp)) {
-            DPRINTF("migration failed: %s\n", strerror(-ret));
-            /* FIXME: we should return meaningful errors */
-            error_set(errp, QERR_UNDEFINED_ERROR);
+    if (ret < 0 || local_err) {
+        if (!local_err) {
+            error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
+        } else {
+            error_propagate(errp, local_err);
         }
         return;
     }
-- 
1.7.12.1

[Qemu-devel] [PATCH 06/18] migration: centralize call to migrate_fd_error()

[Paolo Bonzini]

The call to migrate_fd_error() was missing for non-socket backends, so
centralize it in qmp_migrate().

Before:

    (qemu) migrate fd:ffff
    migrate: An undefined error has occurred
    (qemu) info migrate
    (qemu)

After:

    (qemu) migrate fd:ffff
    migrate: An undefined error has occurred
    (qemu) info migrate
    capabilities: xbzrle: off
    Migration status: failed
    total time: 0 milliseconds

(The awful error message will be fixed later in the series).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration-tcp.c  | 1 -
 migration-unix.c | 1 -
 migration.c      | 1 +
 3 file modificati, 1 inserzione(+), 2 rimozioni(-)

diff --git a/migration-tcp.c b/migration-tcp.c
index 78337a3..e8bc76a 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -79,7 +79,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
 
     s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
     if (local_err != NULL) {
-        migrate_fd_error(s);
         error_propagate(errp, local_err);
         return -1;
     }
diff --git a/migration-unix.c b/migration-unix.c
index 169de88..d349662 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -111,7 +111,6 @@ int unix_start_outgoing_migration(MigrationState *s, const char *path)
 
     if (ret < 0) {
         DPRINTF("connect failed\n");
-        migrate_fd_error(s);
         return ret;
     }
     migrate_fd_connect(s);
diff --git a/migration.c b/migration.c
index 8a04174..a56358e 100644
--- a/migration.c
+++ b/migration.c
@@ -522,6 +522,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     }
 
     if (ret < 0 || local_err) {
+        migrate_fd_error(s);
         if (!local_err) {
             error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
         } else {
-- 
1.7.12.1

[Qemu-devel] [PATCH 07/18] migration: use qemu-sockets to establish Unix sockets

[Paolo Bonzini]

This makes migration-unix.c again a cut-and-paste job from migration-tcp.c,
exactly as it was in the beginning. :)

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration-unix.c | 94 ++++++++++----------------------------------------------
 migration.c      |  4 +--
 migration.h      |  4 +--
 3 file modificati, 21 inserzioni(+), 81 rimozioni(-)

diff --git a/migration-unix.c b/migration-unix.c
index d349662..5387c21 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -53,67 +53,34 @@ static int unix_close(MigrationState *s)
     return r;
 }
 
-static void unix_wait_for_connect(void *opaque)
+static void unix_wait_for_connect(int fd, void *opaque)
 {
     MigrationState *s = opaque;
-    int val, ret;
-    socklen_t valsize = sizeof(val);
 
-    DPRINTF("connect completed\n");
-    do {
-        ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
-    } while (ret == -1 && errno == EINTR);
-
-    if (ret < 0) {
+    if (fd < 0) {
+        DPRINTF("migrate connect error\n");
+        s->fd = -1;
         migrate_fd_error(s);
-        return;
-    }
-
-    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-
-    if (val == 0)
+    } else {
+        DPRINTF("migrate connect success\n");
+        s->fd = fd;
         migrate_fd_connect(s);
-    else {
-        DPRINTF("error connecting %d\n", val);
-        migrate_fd_error(s);
     }
 }
 
-int unix_start_outgoing_migration(MigrationState *s, const char *path)
+int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
 {
-    struct sockaddr_un addr;
-    int ret;
+    Error *local_err = NULL;
 
-    addr.sun_family = AF_UNIX;
-    snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
     s->get_error = unix_errno;
     s->write = unix_write;
     s->close = unix_close;
 
-    s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
-    if (s->fd == -1) {
-        DPRINTF("Unable to open socket");
-        return -errno;
-    }
-
-    socket_set_nonblock(s->fd);
-
-    do {
-        ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1) {
-            ret = -errno;
-        }
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
-	    qemu_set_fd_handler2(s->fd, NULL, NULL, unix_wait_for_connect, s);
-            return 0;
-        }
-    } while (ret == -EINTR);
-
-    if (ret < 0) {
-        DPRINTF("connect failed\n");
-        return ret;
+    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return -1;
     }
-    migrate_fd_connect(s);
     return 0;
 }
 
@@ -151,43 +118,16 @@ out2:
     close(s);
 }
 
-int unix_start_incoming_migration(const char *path)
+int unix_start_incoming_migration(const char *path, Error **errp)
 {
-    struct sockaddr_un addr;
     int s;
-    int ret;
-
-    DPRINTF("Attempting to start an incoming migration\n");
-
-    s = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
-    if (s == -1) {
-        fprintf(stderr, "Could not open unix socket: %s\n", strerror(errno));
-        return -errno;
-    }
-
-    memset(&addr, 0, sizeof(addr));
-    addr.sun_family = AF_UNIX;
-    snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
 
-    unlink(addr.sun_path);
-    if (bind(s, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
-        ret = -errno;
-        fprintf(stderr, "bind(unix:%s): %s\n", addr.sun_path, strerror(errno));
-        goto err;
-    }
-    if (listen(s, 1) == -1) {
-        fprintf(stderr, "listen(unix:%s): %s\n", addr.sun_path,
-                strerror(errno));
-        ret = -errno;
-        goto err;
+    s = unix_listen(path, NULL, 0, errp);
+    if (s < 0) {
+        return -1;
     }
 
     qemu_set_fd_handler2(s, NULL, unix_accept_incoming_migration, NULL,
                          (void *)(intptr_t)s);
-
     return 0;
-
-err:
-    close(s);
-    return ret;
 }
diff --git a/migration.c b/migration.c
index a56358e..767e297 100644
--- a/migration.c
+++ b/migration.c
@@ -75,7 +75,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
     else if (strstart(uri, "exec:", &p))
         ret =  exec_start_incoming_migration(p);
     else if (strstart(uri, "unix:", &p))
-        ret = unix_start_incoming_migration(p);
+        ret = unix_start_incoming_migration(p, errp);
     else if (strstart(uri, "fd:", &p))
         ret = fd_start_incoming_migration(p);
 #endif
@@ -512,7 +512,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (strstart(uri, "exec:", &p)) {
         ret = exec_start_outgoing_migration(s, p);
     } else if (strstart(uri, "unix:", &p)) {
-        ret = unix_start_outgoing_migration(s, p);
+        ret = unix_start_outgoing_migration(s, p, &local_err);
     } else if (strstart(uri, "fd:", &p)) {
         ret = fd_start_outgoing_migration(s, p);
 #endif
diff --git a/migration.h b/migration.h
index a9852fc..e0612a3 100644
--- a/migration.h
+++ b/migration.h
@@ -63,9 +63,9 @@ int tcp_start_incoming_migration(const char *host_port, Error **errp);
 int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
                                  Error **errp);
 
-int unix_start_incoming_migration(const char *path);
+int unix_start_incoming_migration(const char *path, Error **errp);
 
-int unix_start_outgoing_migration(MigrationState *s, const char *path);
+int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
 
 int fd_start_incoming_migration(const char *path);
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

[Paolo Bonzini]

Error propagation is already there for socket backends, but it
is (and remains) incomplete because no Error is passed to the
NonBlockingConnectHandler.

With all protocols understanding Error, the code can be simplified
by removing the return value.

Before:

    (qemu) migrate fd:ffff
    migrate: An undefined error has occurred
    (qemu) info migrate
    (qemu)

After:

    (qemu) migrate fd:ffff
    migrate: File descriptor named 'ffff' has not been found
    (qemu) info migrate
    capabilities: xbzrle: off
    Migration status: failed
    total time: 0 milliseconds

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration-exec.c |  8 +++-----
 migration-fd.c   | 11 ++++-------
 migration-tcp.c  | 13 ++-----------
 migration-unix.c | 11 ++---------
 migration.c      | 17 ++++++-----------
 migration.h      |  9 ++++-----
 6 file modificati, 21 inserzioni(+), 48 rimozioni(-)

diff --git a/migration-exec.c b/migration-exec.c
index 6c97db9..f3baf85 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -60,19 +60,17 @@ static int exec_close(MigrationState *s)
     return ret;
 }
 
-int exec_start_outgoing_migration(MigrationState *s, const char *command)
+void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
 {
     FILE *f;
 
     f = popen(command, "w");
     if (f == NULL) {
-        DPRINTF("Unable to popen exec target\n");
         goto err_after_popen;
     }
 
     s->fd = fileno(f);
     if (s->fd == -1) {
-        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
         goto err_after_open;
     }
 
@@ -85,12 +83,12 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
     s->write = file_write;
 
     migrate_fd_connect(s);
-    return 0;
+    return;
 
 err_after_open:
     pclose(f);
 err_after_popen:
-    return -1;
+    error_setg_errno(errp, errno, "failed to popen the migration target");
 }
 
 static void exec_accept_incoming_migration(void *opaque)
diff --git a/migration-fd.c b/migration-fd.c
index 7335167..938a109 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -73,12 +73,11 @@ static int fd_close(MigrationState *s)
     return 0;
 }
 
-int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
+void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
-    s->fd = monitor_get_fd(cur_mon, fdname, NULL);
+    s->fd = monitor_get_fd(cur_mon, fdname, errp);
     if (s->fd == -1) {
-        DPRINTF("fd_migration: invalid file descriptor identifier\n");
-        goto err_after_get_fd;
+        return;
     }
 
     if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
@@ -91,12 +90,10 @@ int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
     s->close = fd_close;
 
     migrate_fd_connect(s);
-    return 0;
+    return;
 
 err_after_open:
     close(s->fd);
-err_after_get_fd:
-    return -1;
 }
 
 static void fd_accept_incoming_migration(void *opaque)
diff --git a/migration-tcp.c b/migration-tcp.c
index e8bc76a..5e54e68 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -68,22 +68,13 @@ static void tcp_wait_for_connect(int fd, void *opaque)
     }
 }
 
-int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
-                                 Error **errp)
+void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp)
 {
-    Error *local_err = NULL;
-
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
 
-    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return -1;
-    }
-
-    return 0;
+    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp);
 }
 
 static void tcp_accept_incoming_migration(void *opaque)
diff --git a/migration-unix.c b/migration-unix.c
index 5387c21..34a413a 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -68,20 +68,13 @@ static void unix_wait_for_connect(int fd, void *opaque)
     }
 }
 
-int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
+void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
 {
-    Error *local_err = NULL;
-
     s->get_error = unix_errno;
     s->write = unix_write;
     s->close = unix_close;
 
-    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return -1;
-    }
-    return 0;
+    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, errp);
 }
 
 static void unix_accept_incoming_migration(void *opaque)
diff --git a/migration.c b/migration.c
index 767e297..f7f0138 100644
--- a/migration.c
+++ b/migration.c
@@ -485,7 +485,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     MigrationState *s = migrate_get_current();
     MigrationParams params;
     const char *p;
-    int ret;
 
     params.blk = blk;
     params.shared = inc;
@@ -507,27 +506,23 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     s = migrate_init(&params);
 
     if (strstart(uri, "tcp:", &p)) {
-        ret = tcp_start_outgoing_migration(s, p, &local_err);
+        tcp_start_outgoing_migration(s, p, &local_err);
 #if !defined(WIN32)
     } else if (strstart(uri, "exec:", &p)) {
-        ret = exec_start_outgoing_migration(s, p);
+        exec_start_outgoing_migration(s, p, &local_err);
     } else if (strstart(uri, "unix:", &p)) {
-        ret = unix_start_outgoing_migration(s, p, &local_err);
+        unix_start_outgoing_migration(s, p, &local_err);
     } else if (strstart(uri, "fd:", &p)) {
-        ret = fd_start_outgoing_migration(s, p);
+        fd_start_outgoing_migration(s, p, &local_err);
 #endif
     } else {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
         return;
     }
 
-    if (ret < 0 || local_err) {
+    if (local_err) {
         migrate_fd_error(s);
-        if (!local_err) {
-            error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
-        } else {
-            error_propagate(errp, local_err);
-        }
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/migration.h b/migration.h
index e0612a3..275d2d8 100644
--- a/migration.h
+++ b/migration.h
@@ -56,20 +56,19 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);
 
 int exec_start_incoming_migration(const char *host_port);
 
-int exec_start_outgoing_migration(MigrationState *s, const char *host_port);
+void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
 
 int tcp_start_incoming_migration(const char *host_port, Error **errp);
 
-int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
-                                 Error **errp);
+void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
 
 int unix_start_incoming_migration(const char *path, Error **errp);
 
-int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
+void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
 
 int fd_start_incoming_migration(const char *path);
 
-int fd_start_outgoing_migration(MigrationState *s, const char *fdname);
+void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
 
 void migrate_fd_error(MigrationState *s);
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 09/18] migration (incoming): add error propagation for fd and exec protocols

[Paolo Bonzini]

And remove the superfluous integer return value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration-exec.c |  8 +++-----
 migration-fd.c   |  8 +++-----
 migration-tcp.c  |  7 ++-----
 migration-unix.c |  5 ++---
 migration.c      | 15 ++++++---------
 migration.h      | 10 +++++-----
 vl.c             | 16 ++++++----------
 7 file modificati, 27 inserzioni(+), 42 rimozioni(-)

diff --git a/migration-exec.c b/migration-exec.c
index f3baf85..232be6b 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -100,19 +100,17 @@ static void exec_accept_incoming_migration(void *opaque)
     qemu_fclose(f);
 }
 
-int exec_start_incoming_migration(const char *command)
+void exec_start_incoming_migration(const char *command, Error **errp)
 {
     QEMUFile *f;
 
     DPRINTF("Attempting to start an incoming migration\n");
     f = qemu_popen_cmd(command, "r");
     if(f == NULL) {
-        DPRINTF("Unable to apply qemu wrapper to popen file\n");
-        return -errno;
+        error_setg_errno(errp, errno, "failed to popen the migration source");
+        return;
     }
 
     qemu_set_fd_handler2(qemu_stdio_fd(f), NULL,
 			 exec_accept_incoming_migration, NULL, f);
-
-    return 0;
 }
diff --git a/migration-fd.c b/migration-fd.c
index 938a109..24eedd5 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -105,7 +105,7 @@ static void fd_accept_incoming_migration(void *opaque)
     qemu_fclose(f);
 }
 
-int fd_start_incoming_migration(const char *infd)
+void fd_start_incoming_migration(const char *infd, Error **errp)
 {
     int fd;
     QEMUFile *f;
@@ -115,11 +115,9 @@ int fd_start_incoming_migration(const char *infd)
     fd = strtol(infd, NULL, 0);
     f = qemu_fdopen(fd, "rb");
     if(f == NULL) {
-        DPRINTF("Unable to apply qemu wrapper to file descriptor\n");
-        return -errno;
+        error_setg_errno(errp, errno, "failed to open the source descriptor");
+        return;
     }
 
     qemu_set_fd_handler2(fd, NULL, fd_accept_incoming_migration, NULL, f);
-
-    return 0;
 }
diff --git a/migration-tcp.c b/migration-tcp.c
index 5e54e68..46f6ac5 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -111,18 +111,15 @@ out2:
     close(s);
 }
 
-int tcp_start_incoming_migration(const char *host_port, Error **errp)
+void tcp_start_incoming_migration(const char *host_port, Error **errp)
 {
     int s;
 
     s = inet_listen(host_port, NULL, 256, SOCK_STREAM, 0, errp);
-
     if (s < 0) {
-        return -1;
+        return;
     }
 
     qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
                          (void *)(intptr_t)s);
-
-    return 0;
 }
diff --git a/migration-unix.c b/migration-unix.c
index 34a413a..ed3db3a 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -111,16 +111,15 @@ out2:
     close(s);
 }
 
-int unix_start_incoming_migration(const char *path, Error **errp)
+void unix_start_incoming_migration(const char *path, Error **errp)
 {
     int s;
 
     s = unix_listen(path, NULL, 0, errp);
     if (s < 0) {
-        return -1;
+        return;
     }
 
     qemu_set_fd_handler2(s, NULL, unix_accept_incoming_migration, NULL,
                          (void *)(intptr_t)s);
-    return 0;
 }
diff --git a/migration.c b/migration.c
index f7f0138..2802918 100644
--- a/migration.c
+++ b/migration.c
@@ -64,26 +64,23 @@ static MigrationState *migrate_get_current(void)
     return &current_migration;
 }
 
-int qemu_start_incoming_migration(const char *uri, Error **errp)
+void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
-    int ret;
 
     if (strstart(uri, "tcp:", &p))
-        ret = tcp_start_incoming_migration(p, errp);
+        tcp_start_incoming_migration(p, errp);
 #if !defined(WIN32)
     else if (strstart(uri, "exec:", &p))
-        ret =  exec_start_incoming_migration(p);
+        exec_start_incoming_migration(p, errp);
     else if (strstart(uri, "unix:", &p))
-        ret = unix_start_incoming_migration(p, errp);
+        unix_start_incoming_migration(p, errp);
     else if (strstart(uri, "fd:", &p))
-        ret = fd_start_incoming_migration(p);
+        fd_start_incoming_migration(p, errp);
 #endif
     else {
-        fprintf(stderr, "unknown migration protocol: %s\n", uri);
-        ret = -EPROTONOSUPPORT;
+        error_setg(errp, "unknown migration protocol: %s\n", uri);
     }
-    return ret;
 }
 
 void process_incoming_migration(QEMUFile *f)
diff --git a/migration.h b/migration.h
index 275d2d8..aa44e1b 100644
--- a/migration.h
+++ b/migration.h
@@ -46,7 +46,7 @@ struct MigrationState
 
 void process_incoming_migration(QEMUFile *f);
 
-int qemu_start_incoming_migration(const char *uri, Error **errp);
+void qemu_start_incoming_migration(const char *uri, Error **errp);
 
 uint64_t migrate_max_downtime(void);
 
@@ -54,19 +54,19 @@ void do_info_migrate_print(Monitor *mon, const QObject *data);
 
 void do_info_migrate(Monitor *mon, QObject **ret_data);
 
-int exec_start_incoming_migration(const char *host_port);
+void exec_start_incoming_migration(const char *host_port, Error **errp);
 
 void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
 
-int tcp_start_incoming_migration(const char *host_port, Error **errp);
+void tcp_start_incoming_migration(const char *host_port, Error **errp);
 
 void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
 
-int unix_start_incoming_migration(const char *path, Error **errp);
+void unix_start_incoming_migration(const char *path, Error **errp);
 
 void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
 
-int fd_start_incoming_migration(const char *path);
+void fd_start_incoming_migration(const char *path, Error **errp);
 
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
 
diff --git a/vl.c b/vl.c
index 8d305ca..53917c9 100644
--- a/vl.c
+++ b/vl.c
@@ -3747,16 +3747,12 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (incoming) {
-        Error *errp = NULL;
-        int ret = qemu_start_incoming_migration(incoming, &errp);
-        if (ret < 0) {
-            if (error_is_set(&errp)) {
-                fprintf(stderr, "Migrate: %s\n", error_get_pretty(errp));
-                error_free(errp);
-            }
-            fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
-                    incoming, ret);
-            exit(ret);
+        Error *local_err = NULL;
+        qemu_start_incoming_migration(incoming, &local_err);
+        if (local_err) {
+            fprintf(stderr, "-incoming %s: %s\n", incoming, error_get_pretty(local_err));
+            error_free(local_err);
+            exit(1);
         }
     } else if (autostart) {
         vm_start();
-- 
1.7.12.1

[Qemu-devel] [PATCH 10/18] qemu-char: ask and print error information from qemu-sockets

[Paolo Bonzini]

Before:

    $ qemu-system-x86_64 -monitor tcp:localhost:6000
    (starts despite error)

    $ qemu-system-x86_64 -monitor tcp:foo.bar:12345
    getaddrinfo(foo.bar,12345): Name or service not known
    chardev: opening backend "socket" failed

    $ qemu-system-x86_64 -monitor tcp:localhost:443,server=on
    inet_listen_opts: bind(ipv4,127.0.0.1,443): Permission denied
    inet_listen_opts: FAILED
    chardev: opening backend "socket" failed

After:

    $ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:localhost:6000
    x86_64-softmmu/qemu-system-x86_64: -monitor tcp:localhost:6000: Failed to connect to socket: Connection refused
    chardev: opening backend "socket" failed

    $ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:foo.bar:12345
    qemu-system-x86_64: -monitor tcp:foo.bar:12345: address resolution failed for foo.bar:12345: Name or service not known
    chardev: opening backend "socket" failed

    $ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:localhost:443,server=on
    qemu-system-x86_64: -monitor tcp:localhost:443,server=on: Failed to bind socket: Permission denied
    chardev: opening backend "socket" failed

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c | 23 +++++++++++++++++------
 1 file modificato, 17 inserzioni(+), 6 rimozioni(-)

diff --git a/qemu-char.c b/qemu-char.c
index 8ebd582..04b5c23 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2097,12 +2097,13 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
     NetCharDriver *s = NULL;
+    Error *local_err = NULL;
     int fd = -1;
 
     chr = g_malloc0(sizeof(CharDriverState));
     s = g_malloc0(sizeof(NetCharDriver));
 
-    fd = inet_dgram_opts(opts, NULL);
+    fd = inet_dgram_opts(opts, &local_err);
     if (fd < 0) {
         fprintf(stderr, "inet_dgram_opts failed\n");
         goto return_err;
@@ -2118,6 +2119,10 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
     return chr;
 
 return_err:
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
     g_free(chr);
     g_free(s);
     if (fd >= 0) {
@@ -2428,6 +2433,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 {
     CharDriverState *chr = NULL;
     TCPCharDriver *s = NULL;
+    Error *local_err = NULL;
     int fd = -1;
     int is_listen;
     int is_waitconnect;
@@ -2448,15 +2454,15 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
 
     if (is_unix) {
         if (is_listen) {
-            fd = unix_listen_opts(opts, NULL);
+            fd = unix_listen_opts(opts, &local_err);
         } else {
-            fd = unix_connect_opts(opts, NULL, NULL, NULL);
+            fd = unix_connect_opts(opts, &local_err, NULL, NULL);
         }
     } else {
         if (is_listen) {
-            fd = inet_listen_opts(opts, 0, NULL);
+            fd = inet_listen_opts(opts, 0, &local_err);
         } else {
-            fd = inet_connect_opts(opts, NULL, NULL, NULL);
+            fd = inet_connect_opts(opts, &local_err, NULL, NULL);
         }
     }
     if (fd < 0) {
@@ -2517,8 +2523,13 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     return chr;
 
  fail:
-    if (fd >= 0)
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    if (fd >= 0) {
         closesocket(fd);
+    }
     g_free(s);
     g_free(chr);
     return NULL;
-- 
1.7.12.1

[Qemu-devel] [PATCH 11/18] nbd: ask and print error information from qemu-sockets

[Paolo Bonzini]

Before:

    $ qemu-system-x86_64 nbd:localhost:12345
    inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
    qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused

After:

    $ x86_64-softmmu/qemu-system-x86_64 nbd:localhost:12345
    qemu-system-x86_64: Failed to connect to socket: Connection refused
    qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 nbd.c | 39 +++++++++++++++++++++++++++++++--------
 1 file modificato, 31 inserzioni(+), 8 rimozioni(-)

diff --git a/nbd.c b/nbd.c
index f61a288..cec5a94 100644
--- a/nbd.c
+++ b/nbd.c
@@ -208,7 +208,14 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
 
 int tcp_socket_outgoing_spec(const char *address_and_port)
 {
-    return inet_connect(address_and_port, NULL);
+    Error *local_err = NULL;
+    int fd = inet_connect(address_and_port, &local_err);
+
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
 }
 
 int tcp_socket_incoming(const char *address, uint16_t port)
@@ -220,22 +227,38 @@ int tcp_socket_incoming(const char *address, uint16_t port)
 
 int tcp_socket_incoming_spec(const char *address_and_port)
 {
-    char *ostr  = NULL;
-    int olen = 0;
-    return inet_listen(address_and_port, ostr, olen, SOCK_STREAM, 0, NULL);
+    Error *local_err = NULL;
+    int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
+
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
 }
 
 int unix_socket_incoming(const char *path)
 {
-    char *ostr = NULL;
-    int olen = 0;
+    Error *local_err = NULL;
+    int fd = unix_listen(path, NULL, 0, &local_err);
 
-    return unix_listen(path, ostr, olen, NULL);
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
 }
 
 int unix_socket_outgoing(const char *path)
 {
-    return unix_connect(path, NULL);
+    Error *local_err = NULL;
+    int fd = unix_connect(path, &local_err);
+
+    if (local_err != NULL) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return fd;
 }
 
 /* Basic flow for negotiation
-- 
1.7.12.1

[Qemu-devel] [PATCH 12/18] qemu-ga: ask and print error information from qemu-sockets

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qga/channel-posix.c | 8 +++++---
 1 file modificato, 5 inserzioni(+), 3 rimozioni(-)

diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index e22eee6..d152827 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -181,9 +181,11 @@ 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), NULL);
-        if (fd == -1) {
-            g_critical("error opening path: %s", strerror(errno));
+        Error *local_err = NULL;
+        int fd = unix_listen(path, NULL, strlen(path), &local_err);
+        if (local_err != NULL) {
+            g_critical("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return false;
         }
         ga_channel_listen_add(c, fd, true);
-- 
1.7.12.1

[Qemu-devel] [PATCH 13/18] vnc: add error propagation to vnc_display_open

[Paolo Bonzini]

Before:

    $ qemu-system-x86_64 -vnc foo.bar:12345
    getaddrinfo(foo.bar,18245): Name or service not known
    Failed to start VNC server on `foo.bar:12345'

    $ qemu-system-x86_64 -vnc localhost:12345,reverse=on
    inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
    Failed to start VNC server on `localhost:12345,reverse=on'

After:

    $ x86_64-softmmu/qemu-system-x86_64 -vnc foo.bar:12345
    qemu-system-x86_64: address resolution failed for foo.bar:18245: Name or service not known
    Failed to start VNC server on `foo.bar:12345'

    $ x86_64-softmmu/qemu-system-x86_64 -vnc localhost:12345,reverse=on
    qemu-system-x86_64: Failed to connect to socket: Connection refused
    Failed to start VNC server on `localhost:12345,reverse=on'

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 console.h |  2 +-
 qmp.c     |  6 ++----
 ui/vnc.c  | 67 +++++++++++++++++++++++++++++----------------------------------
 vl.c      |  5 ++++-
 4 file modificati, 38 inserzioni(+), 42 rimozioni(-)

diff --git a/console.h b/console.h
index f990684..fcf2fc5 100644
--- a/console.h
+++ b/console.h
@@ -378,7 +378,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
 /* vnc.c */
 void vnc_display_init(DisplayState *ds);
 void vnc_display_close(DisplayState *ds);
-int vnc_display_open(DisplayState *ds, const char *display);
+int vnc_display_open(DisplayState *ds, const char *display, Error **errp);
 void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
 int vnc_display_disable_login(DisplayState *ds);
 char *vnc_display_local_addr(DisplayState *ds);
diff --git a/qmp.c b/qmp.c
index 36c54c5..31bc3bf 100644
--- a/qmp.c
+++ b/qmp.c
@@ -349,11 +349,9 @@ void qmp_change_vnc_password(const char *password, Error **errp)
     }
 }
 
-static void qmp_change_vnc_listen(const char *target, Error **err)
+static void qmp_change_vnc_listen(const char *target, Error **errp)
 {
-    if (vnc_display_open(NULL, target) < 0) {
-        error_set(err, QERR_VNC_SERVER_FAILED, target);
-    }
+    vnc_display_open(NULL, target, errp);
 }
 
 static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
diff --git a/ui/vnc.c b/ui/vnc.c
index 235596e..0d92670 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2844,7 +2844,7 @@ char *vnc_display_local_addr(DisplayState *ds)
     return vnc_socket_local_addr("%s:%s", vs->lsock);
 }
 
-int vnc_display_open(DisplayState *ds, const char *display)
+int vnc_display_open(DisplayState *ds, const char *display, Error **errp)
 {
     VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
     const char *options;
@@ -2863,13 +2863,12 @@ int vnc_display_open(DisplayState *ds, const char *display)
     int lock_key_sync = 1;
 
     if (!vnc_display)
-        return -1;
+        goto fail;
     vnc_display_close(ds);
     if (strcmp(display, "none") == 0)
         return 0;
 
-    if (!(vs->display = strdup(display)))
-        return -1;
+    vs->display = g_strdup(display);
     vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
 
     options = display;
@@ -2877,13 +2876,11 @@ int vnc_display_open(DisplayState *ds, const char *display)
         options++;
         if (strncmp(options, "password", 8) == 0) {
             if (fips_get_state()) {
-                fprintf(stderr,
-                        "VNC password auth disabled due to FIPS mode, "
-                        "consider using the VeNCrypt or SASL authentication "
-                        "methods as an alternative\n");
-                g_free(vs->display);
-                vs->display = NULL;
-                return -1;
+                error_setg(errp,
+                           "VNC password auth disabled due to FIPS mode, "
+                           "consider using the VeNCrypt or SASL authentication "
+                           "methods as an alternative\n");
+                goto fail;
             }
             password = 1; /* Require password auth */
         } else if (strncmp(options, "reverse", 7) == 0) {
@@ -2913,18 +2910,14 @@ int vnc_display_open(DisplayState *ds, const char *display)
 
                 VNC_DEBUG("Trying certificate path '%s'\n", path);
                 if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
-                    fprintf(stderr, "Failed to find x509 certificates/keys in %s\n", path);
+                    error_setg(errp, "Failed to find x509 certificates/keys in %s\n", path);
                     g_free(path);
-                    g_free(vs->display);
-                    vs->display = NULL;
-                    return -1;
+                    goto fail;
                 }
                 g_free(path);
             } else {
-                fprintf(stderr, "No certificate path provided\n");
-                g_free(vs->display);
-                vs->display = NULL;
-                return -1;
+                error_setg(errp, "No certificate path provided\n");
+                goto fail;
             }
 #endif
 #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
@@ -2943,10 +2936,8 @@ int vnc_display_open(DisplayState *ds, const char *display)
             } else if (strncmp(options+6, "force-shared", 12) == 0) {
                 vs->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
             } else {
-                fprintf(stderr, "unknown vnc share= option\n");
-                g_free(vs->display);
-                vs->display = NULL;
-                return -1;
+                error_setg(errp, "unknown vnc share= option\n");
+                goto fail;
             }
         }
     }
@@ -3047,11 +3038,9 @@ int vnc_display_open(DisplayState *ds, const char *display)
 
 #ifdef CONFIG_VNC_SASL
     if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
-        fprintf(stderr, "Failed to initialize SASL auth %s",
-                sasl_errstring(saslErr, NULL, NULL));
-        g_free(vs->display);
-        vs->display = NULL;
-        return -1;
+        error_setg(errp, "Failed to initialize SASL auth %s",
+                   sasl_errstring(saslErr, NULL, NULL));
+        goto fail;
     }
 #endif
     vs->lock_key_sync = lock_key_sync;
@@ -3059,13 +3048,11 @@ 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, NULL);
+            vs->lsock = unix_connect(display+5, errp);
         else
-            vs->lsock = inet_connect(display, NULL);
+            vs->lsock = inet_connect(display, errp);
         if (-1 == vs->lsock) {
-            g_free(vs->display);
-            vs->display = NULL;
-            return -1;
+            goto fail;
         } else {
             int csock = vs->lsock;
             vs->lsock = -1;
@@ -3079,20 +3066,28 @@ 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, NULL);
+            vs->lsock = unix_listen(display+5, dpy+5, 256-5, errp);
         } else {
             vs->lsock = inet_listen(display, dpy, 256,
-                                    SOCK_STREAM, 5900, NULL);
+                                    SOCK_STREAM, 5900, errp);
         }
         if (-1 == vs->lsock) {
             g_free(dpy);
-            return -1;
+            goto fail;
         } else {
             g_free(vs->display);
             vs->display = dpy;
         }
     }
     return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
+
+fail:
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_VNC_SERVER_FAILED, display);
+    }
+    g_free(vs->display);
+    vs->display = NULL;
+    return -1;
 }
 
 void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
diff --git a/vl.c b/vl.c
index 53917c9..45a5ba5 100644
--- a/vl.c
+++ b/vl.c
@@ -3692,8 +3692,11 @@ int main(int argc, char **argv, char **envp)
 #ifdef CONFIG_VNC
     /* init remote displays */
     if (vnc_display) {
+        Error *local_err = NULL;
         vnc_display_init(ds);
-        if (vnc_display_open(ds, vnc_display) < 0) {
+        if (vnc_display_open(ds, vnc_display, &local_err) < 0) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             fprintf(stderr, "Failed to start VNC server on `%s'\n",
                     vnc_display);
             exit(1);
-- 
1.7.12.1

[Qemu-devel] [PATCH 14/18] qemu-sockets: include strerror or gai_strerror output in error messages

[Paolo Bonzini]

Among others, before:

    $ qemu-system-x86_64 -chardev socket,port=12345,id=char
    inet_connect: host and/or port not specified
    chardev: opening backend "socket" failed

After:

    $ x86_64-softmmu/qemu-system-x86_64 -chardev socket,port=12345,id=char
    qemu-system-x86_64: -chardev socket,port=12345,id=char: host and/or port not specified
    chardev: opening backend "socket" failed

perror and fprintf can be removed because all clients can now
consume Errors properly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 30 +++++++++---------------------
 1 file modificato, 9 inserzioni(+), 21 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 341ae21..8ab2d0c 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -120,8 +120,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
 
     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_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_setg(errp, "host and/or port not specified");
         return -1;
     }
     pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port"));
@@ -138,9 +137,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
         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_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+                   gai_strerror(rc));
         return -1;
     }
 
@@ -151,10 +149,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
 		        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));
             if (!e->ai_next) {
-                error_set(errp, QERR_SOCKET_CREATE_FAILED);
+                error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
             }
             continue;
         }
@@ -176,24 +172,19 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
                 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));
                 if (!e->ai_next) {
-                    error_set(errp, QERR_SOCKET_BIND_FAILED);
+                    error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
                 }
             }
         }
         closesocket(slisten);
     }
-    fprintf(stderr, "%s: FAILED\n", __FUNCTION__);
     freeaddrinfo(res);
     return -1;
 
 listen:
     if (listen(slisten,1) != 0) {
-        error_set(errp, QERR_SOCKET_LISTEN_FAILED);
-        perror("listen");
+        error_set_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
         closesocket(slisten);
         freeaddrinfo(res);
         return -1;
@@ -325,9 +316,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
     addr = qemu_opt_get(opts, "host");
     port = qemu_opt_get(opts, "port");
     if (addr == NULL || port == NULL) {
-        fprintf(stderr,
-                "inet_parse_connect_opts: host and/or port not specified\n");
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_setg(errp, "host and/or port not specified");
         return NULL;
     }
 
@@ -341,9 +330,8 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
     /* lookup */
     rc = getaddrinfo(addr, port, &ai, &res);
     if (rc != 0) {
-        fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
-                gai_strerror(rc));
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+                   gai_strerror(rc));
         return NULL;
     }
     return res;
-- 
1.7.12.1

[Qemu-devel] [PATCH 15/18] qemu-sockets: add error propagation to inet_connect_addr

[Paolo Bonzini]

perror and fprintf can be removed because all clients can now consume
Errors properly.  However, we need to change the non-blocking connect
handlers to take an Error, in order to improve error handling for
migration with the TCP protocol.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 15 ++++++---------
 1 file modificato, 6 inserzioni(+), 9 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 8ab2d0c..b096f49 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -216,7 +216,7 @@ typedef struct ConnectState {
 } ConnectState;
 
 static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
-                             ConnectState *connect_state);
+                             ConnectState *connect_state, Error **errp);
 
 static void wait_for_connect(void *opaque)
 {
@@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque)
     if (s->current_addr) {
         while (s->current_addr->ai_next != NULL && s->fd < 0) {
             s->current_addr = s->current_addr->ai_next;
-            s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
+            s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
             /* connect in progress */
             if (in_progress) {
                 return;
@@ -264,7 +264,7 @@ static void wait_for_connect(void *opaque)
 }
 
 static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
-                             ConnectState *connect_state)
+                             ConnectState *connect_state, Error **errp)
 {
     int sock, rc;
 
@@ -272,8 +272,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
 
     sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
     if (sock < 0) {
-        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
-                inet_strfamily(addr->ai_family), strerror(errno));
+        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
     setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
@@ -294,6 +293,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
                              connect_state);
         *in_progress = true;
     } else if (rc < 0) {
+        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
         closesocket(sock);
         return -1;
     }
@@ -376,7 +376,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
         if (connect_state != NULL) {
             connect_state->current_addr = e;
         }
-        sock = inet_connect_addr(e, &in_progress, connect_state);
+        sock = inet_connect_addr(e, &in_progress, connect_state, errp);
         if (in_progress) {
             return sock;
         } else if (sock >= 0) {
@@ -387,9 +387,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
             break;
         }
     }
-    if (sock < 0) {
-        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
-    }
     g_free(connect_state);
     freeaddrinfo(res);
     return sock;
-- 
1.7.12.1

[Qemu-devel] [PATCH 16/18] qemu-sockets: add error propagation to inet_dgram_opts

[Paolo Bonzini]

Before:

    $ qemu-system-x86_64 -monitor udp:localhost:631@localhost:631
    inet_dgram_opts: bind(ipv4,127.0.0.1,631): OK
    inet_dgram_opts failed
    chardev: opening backend "udp" failed

After:

    $ x86_64-softmmu/qemu-system-x86_64 -monitor udp:localhost:631@localhost:631
    qemu-system-x86_64: -monitor udp:localhost:631@localhost:631: Failed to bind socket: Address already in use
    chardev: opening backend "udp" failed

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c    |  1 -
 qemu-sockets.c | 34 ++++++++--------------------------
 2 file modificati, 8 inserzioni(+), 27 rimozioni(-)

diff --git a/qemu-char.c b/qemu-char.c
index 04b5c23..afe2bfb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2105,7 +2105,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
 
     fd = inet_dgram_opts(opts, &local_err);
     if (fd < 0) {
-        fprintf(stderr, "inet_dgram_opts failed\n");
         goto return_err;
     }
 
diff --git a/qemu-sockets.c b/qemu-sockets.c
index b096f49..6f13121 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -397,8 +397,6 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
     struct addrinfo ai, *peer = NULL, *local = NULL;
     const char *addr;
     const char *port;
-    char uaddr[INET6_ADDRSTRLEN+1];
-    char uport[33];
     int sock = -1, rc;
 
     /* lookup peer addr */
@@ -413,7 +411,7 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
         addr = "localhost";
     }
     if (port == NULL || strlen(port) == 0) {
-        fprintf(stderr, "inet_dgram: port not specified\n");
+        error_setg(errp, "remote port not specified");
         return -1;
     }
 
@@ -423,8 +421,8 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
         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_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+                   gai_strerror(rc));
 	return -1;
     }
 
@@ -443,44 +441,28 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
         port = "0";
 
     if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) {
-        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
-                gai_strerror(rc));
+        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+                   gai_strerror(rc));
         goto err;
     }
 
     /* 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));
+        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         goto err;
     }
     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));
+        error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
         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_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
         goto err;
     }
 
-- 
1.7.12.1

[Qemu-devel] [PATCH 17/18] qemu-sockets: add error propagation to inet_parse

[Paolo Bonzini]

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 41 +++++++++++++++++++++--------------------
 1 file modificato, 21 inserzioni(+), 20 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6f13121..55669e9 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -481,7 +481,7 @@ err:
 }
 
 /* compatibility wrapper */
-static int inet_parse(QemuOpts *opts, const char *str)
+static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
 {
     const char *optstr, *h;
     char addr[64];
@@ -493,32 +493,28 @@ static int inet_parse(QemuOpts *opts, const char *str)
         /* no host given */
         addr[0] = '\0';
         if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
-            fprintf(stderr, "%s: portonly parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing port in address '%s'", str);
+            return;
         }
     } else if (str[0] == '[') {
         /* IPv6 addr */
         if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
-            fprintf(stderr, "%s: ipv6 parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing IPv6 address '%s'", str);
+            return;
         }
         qemu_opt_set(opts, "ipv6", "on");
     } else if (qemu_isdigit(str[0])) {
         /* IPv4 addr */
         if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
-            fprintf(stderr, "%s: ipv4 parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing IPv4 address '%s'", str);
+            return;
         }
         qemu_opt_set(opts, "ipv4", "on");
     } else {
         /* hostname */
         if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
-            fprintf(stderr, "%s: hostname parse error (%s)\n",
-                    __FUNCTION__, str);
-            return -1;
+            error_setg(errp, "error parsing address '%s'", str);
+            return;
         }
     }
     qemu_opt_set(opts, "host", addr);
@@ -533,7 +529,6 @@ static int inet_parse(QemuOpts *opts, const char *str)
         qemu_opt_set(opts, "ipv4", "on");
     if (strstr(optstr, ",ipv6"))
         qemu_opt_set(opts, "ipv6", "on");
-    return 0;
 }
 
 int inet_listen(const char *str, char *ostr, int olen,
@@ -542,9 +537,11 @@ int inet_listen(const char *str, char *ostr, int olen,
     QemuOpts *opts;
     char *optstr;
     int sock = -1;
+    Error *local_err = NULL;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    if (inet_parse(opts, str) == 0) {
+    inet_parse(opts, str, &local_err);
+    if (local_err == NULL) {
         sock = inet_listen_opts(opts, port_offset, errp);
         if (sock != -1 && ostr) {
             optstr = strchr(str, ',');
@@ -561,7 +558,7 @@ int inet_listen(const char *str, char *ostr, int olen,
             }
         }
     } else {
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_propagate(errp, local_err);
     }
     qemu_opts_del(opts);
     return sock;
@@ -579,12 +576,14 @@ int inet_connect(const char *str, Error **errp)
 {
     QemuOpts *opts;
     int sock = -1;
+    Error *local_err = NULL;
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    if (inet_parse(opts, str) == 0) {
+    inet_parse(opts, str, &local_err);
+    if (local_err == NULL) {
         sock = inet_connect_opts(opts, errp, NULL, NULL);
     } else {
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_propagate(errp, local_err);
     }
     qemu_opts_del(opts);
     return sock;
@@ -609,14 +608,16 @@ int inet_nonblocking_connect(const char *str,
 {
     QemuOpts *opts;
     int sock = -1;
+    Error *local_err = NULL;
 
     g_assert(callback != NULL);
 
     opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
-    if (inet_parse(opts, str) == 0) {
+    inet_parse(opts, str, &local_err);
+    if (local_err == NULL) {
         sock = inet_connect_opts(opts, errp, callback, opaque);
     } else {
-        error_set(errp, QERR_SOCKET_CREATE_FAILED);
+        error_propagate(errp, local_err);
     }
     qemu_opts_del(opts);
     return sock;
-- 
1.7.12.1

[Qemu-devel] [PATCH 18/18] qemu-sockets: add error propagation to Unix socket functions

[Paolo Bonzini]

Before:

    $ qemu-system-x86_64 -monitor unix:/vvv,server=off
    connect(unix:/vvv): No such file or directory
    chardev: opening backend "socket" failed

After:

    $ x86_64-softmmu/qemu-system-x86_64 -monitor unix:/vvv,server=off
    qemu-system-x86_64: -monitor unix:/vvv,server=off: Failed to connect to socket: No such file or directory
    chardev: opening backend "socket" failed

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-sockets.c | 12 ++++++------
 1 file modificato, 6 inserzioni(+), 6 rimozioni(-)

diff --git a/qemu-sockets.c b/qemu-sockets.c
index 55669e9..e28c63c 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -633,7 +633,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
-        perror("socket(unix)");
+        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
 
@@ -658,11 +658,11 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
 
     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_errno(errp, errno, 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_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
         goto err;
     }
 
@@ -682,7 +682,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
     int sock, rc;
 
     if (NULL == path) {
-        fprintf(stderr, "unix connect: no path specified\n");
+        error_setg(errp, "unix connect: no path specified\n");
         return -1;
     }
 
@@ -694,7 +694,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
 
     sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
-        perror("socket(unix)");
+        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
         return -1;
     }
     if (connect_state != NULL) {
@@ -726,7 +726,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
     }
 
     if (rc < 0) {
-        fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
+        error_set_errno(errp, -rc, QERR_SOCKET_CONNECT_FAILED);
         close(sock);
 	return -1;
     }
-- 
1.7.12.1

Re: [Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:48 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> These functions help maintaining homogeneous formatting of error
> messages that include strerror values.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

One small comment below.

> ---
>  error.c | 28 ++++++++++++++++++++++++++++
>  error.h |  9 +++++++++
>  2 file modificati, 37 inserzioni(+)
> 
> diff --git a/error.c b/error.c
> index 1f05fc4..128d88c 100644
> --- a/error.c
> +++ b/error.c
> @@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      *errp = err;
>  }
>  
> +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> +                     const char *fmt, ...)

The function's name makes me expect that something else is done with os_errno
other than appending the strerror() string to the user message, but I can't
suggest anything better (and allows for storing errno in the Error object in
the future if we want to).

> +{
> +    Error *err;
> +    char *msg1;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +    assert(*errp == NULL);
> +
> +    err = g_malloc0(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    msg1 = g_strdup_vprintf(fmt, ap);
> +    if (os_errno != 0) {
> +        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
> +        g_free(msg1);
> +    } else {
> +        err->msg = msg1;
> +    }
> +    va_end(ap);
> +    err->err_class = err_class;
> +
> +    *errp = err;
> +}
> +
>  Error *error_copy(const Error *err)
>  {
>      Error *err_new;
> diff --git a/error.h b/error.h
> index da7fed3..4d52e73 100644
> --- a/error.h
> +++ b/error.h
> @@ -30,10 +30,19 @@ typedef struct Error Error;
>  void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>  
>  /**
> + * Set an indirect pointer to an error given a ErrorClass value and a
> + * printf-style human message, followed by a strerror() string if
> + * @os_error is not zero.
> + */
> +void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
> +
> +/**
>   * Same as error_set(), but sets a generic error
>   */
>  #define error_setg(err, fmt, ...) \
>      error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
> +#define error_setg_errno(err, os_error, fmt, ...) \
> +    error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
>  
>  /**
>   * Returns true if an indirect pointer to an error is pointing to a valid

Re: [Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Paolo Bonzini]

Il 04/10/2012 18:14, Luiz Capitulino ha scritto:
>> > +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>> > +                     const char *fmt, ...)
> 
> The function's name makes me expect that something else is done with os_errno
                                                     ^^^^

Why something "else"? :)

> other than appending the strerror() string to the user message, but I can't
> suggest anything better (and allows for storing errno in the Error object in
> the future if we want to).

Who knows... :)

Paolo

Re: [Qemu-devel] [PATCH 02/18] qemu-sockets: add Error ** to all functions

[Luiz Capitulino]

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

Re: [Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Luiz Capitulino]

On Thu, 04 Oct 2012 18:16:13 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/10/2012 18:14, Luiz Capitulino ha scritto:
> >> > +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> >> > +                     const char *fmt, ...)
> > 
> > The function's name makes me expect that something else is done with os_errno
>                                                      ^^^^
> 
> Why something "else"? :)

What I meant is that, it's not clear from the function's name how os_errno
is used. Actually, it gives the impression you're storing errno, but I might
be biased :)

But again, I don't have any better suggestions.

Re: [Qemu-devel] [PATCH 03/18] qemu-sockets: unix_listen and unix_connect are portable

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:50 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> They are just wrappers and do not need a Win32-specific version.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-sockets.c | 49 +++++++++++++++++--------------------------------
>  1 file modificato, 17 inserzioni(+), 32 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 5cf2b32..79c7b66 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -731,6 +731,23 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
>      return sock;
>  }
>  
> +#else
> +
> +int unix_listen_opts(QemuOpts *opts, Error **errp)
> +{
> +    error_setg(errp, "unix sockets are not available on windows");
> +    errno = ENOTSUP;
> +    return -1;
> +}
> +
> +int unix_connect_opts(QemuOpts *opts, Error **errp)
> +{
> +    error_setg(errp, "unix sockets are not available on windows");
> +    errno = ENOTSUP;
> +    return -1;
> +}
> +#endif
> +
>  /* compatibility wrapper */
>  int unix_listen(const char *str, char *ostr, int olen, Error **errp)
>  {
> @@ -773,38 +790,6 @@ int unix_connect(const char *path, Error **errp)
>      return sock;
>  }
>  
> -#else
> -
> -int unix_listen_opts(QemuOpts *opts, Error **errp)
> -{
> -    error_setg(errp, "unix sockets are not available on windows");
> -    errno = ENOTSUP;
> -    return -1;
> -}
> -
> -int unix_connect_opts(QemuOpts *opts, Error **errp)
> -{
> -    error_setg(errp, "unix sockets are not available on windows");
> -    errno = ENOTSUP;
> -    return -1;
> -}
> -
> -int unix_listen(const char *path, char *ostr, int olen, Error **errp)
> -{
> -    error_setg(errp, "unix sockets are not available on windows");
> -    errno = ENOTSUP;
> -    return -1;
> -}
> -
> -int unix_connect(const char *path, Error **errp)
> -{
> -    error_setg(errp, "unix sockets are not available on windows");
> -    errno = ENOTSUP;
> -    return -1;
> -}
> -
> -#endif
> -
>  #ifdef _WIN32
>  static void socket_cleanup(void)
>  {

Re: [Qemu-devel] [PATCH 02/18] qemu-sockets: add Error ** to all functions

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 02/18] qemu-sockets: add Error ** to all functions

[Luiz Capitulino]

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>

Re: [Qemu-devel] [PATCH 04/18] qemu-sockets: add nonblocking connect for Unix sockets

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:51 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

This looks like a bug fix, because if is_waitconnect==false unix_connect_opts()
shouldn't block. Am I right?

Somme comments below.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-char.c    |  2 +-
>  qemu-sockets.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++---------
>  qemu_socket.h  |  6 ++++-
>  3 file modificati, 72 inserzioni(+), 14 rimozioni(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 3cc6cb5..8ebd582 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2450,7 +2450,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>          if (is_listen) {
>              fd = unix_listen_opts(opts, NULL);
>          } else {
> -            fd = unix_connect_opts(opts, NULL);
> +            fd = unix_connect_opts(opts, NULL, NULL, NULL);
>          }
>      } else {
>          if (is_listen) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 79c7b66..341ae21 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -252,16 +252,19 @@ static void wait_for_connect(void *opaque)
>      }
>  
>      /* try to connect to the next address on the list */
> -    while (s->current_addr->ai_next != NULL && s->fd < 0) {
> -        s->current_addr = s->current_addr->ai_next;
> -        s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> -        /* connect in progress */
> -        if (in_progress) {
> -            return;
> +    if (s->current_addr) {
> +        while (s->current_addr->ai_next != NULL && s->fd < 0) {
> +            s->current_addr = s->current_addr->ai_next;
> +            s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> +            /* connect in progress */
> +            if (in_progress) {
> +                return;
> +            }
>          }
> +
> +        freeaddrinfo(s->addr_list);
>      }
>  
> -    freeaddrinfo(s->addr_list);
>      if (s->callback) {
>          s->callback(s->fd, s->opaque);
>      }
> @@ -702,32 +705,65 @@ err:
>      return -1;
>  }
>  
> -int unix_connect_opts(QemuOpts *opts, Error **errp)
> +int unix_connect_opts(QemuOpts *opts, Error **errp,
> +                      NonBlockingConnectHandler *callback, void *opaque)

I wonder if having two functions (one non-blocking and another blocking)
would be better, as we have some "if (non-blocking) do; else" kind of tests.

>  {
>      struct sockaddr_un un;
>      const char *path = qemu_opt_get(opts, "path");
> -    int sock;
> +    ConnectState *connect_state = NULL;
> +    int sock, rc;
>  
>      if (NULL == path) {
>          fprintf(stderr, "unix connect: no path specified\n");
>          return -1;
>      }
>  
> +    if (callback != NULL) {
> +        connect_state = g_malloc0(sizeof(*connect_state));
> +        connect_state->callback = callback;
> +        connect_state->opaque = opaque;
> +    }
> +
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
>          perror("socket(unix)");
>          return -1;
>      }

Leaks connect_state on error, you could delay its allocation.

> +    if (connect_state != NULL) {
> +        socket_set_nonblock(sock);
> +    }
>  
>      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) {
> +
> +    /* connect to peer */
> +    do {
> +        rc = 0;
> +        if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
> +            rc = -socket_error();
> +        }
> +    } while (rc == -EINTR);

Bug fix, woud be nice to comment in the changelog.

> +
> +    if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> +        connect_state->fd = sock;
> +        qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
> +                             connect_state);
> +        return sock;
> +    } else {
> +        /* non blocking socket immediate success, call callback */
> +        if (callback != NULL) {
> +            callback(sock, opaque);
> +        }
> +    }

Wouldn't this be better written like:

if (connect_state) {
	if (rc == 0) {
		/* immediate success */
		callack(sock, opaque);
	} else if (QEMU_SOCKET_RC_INPROGRESS(rc)) {
		connect_state->fd = sock;
		qemu_set_fd_handler2(...);
		return sock;
	}
}

> +
> +    if (rc < 0) {
>          fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
>          close(sock);
>  	return -1;
>      }

leaks connect_state.

>  
> +    g_free(connect_state);
>      return sock;
>  }
>  
> @@ -740,7 +776,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>      return -1;
>  }
>  
> -int unix_connect_opts(QemuOpts *opts, Error **errp)
> +int unix_connect_opts(QemuOpts *opts, Error **errp,
> +                      NonBlockingConnectHandler *callback, void *opaque)
>  {
>      error_setg(errp, "unix sockets are not available on windows");
>      errno = ENOTSUP;
> @@ -785,7 +822,24 @@ int unix_connect(const char *path, Error **errp)
>  
>      opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
>      qemu_opt_set(opts, "path", path);
> -    sock = unix_connect_opts(opts, errp);
> +    sock = unix_connect_opts(opts, errp, NULL, NULL);
> +    qemu_opts_del(opts);
> +    return sock;
> +}
> +
> +
> +int unix_nonblocking_connect(const char *path,
> +                             NonBlockingConnectHandler *callback,
> +                             void *opaque, Error **errp)
> +{
> +    QemuOpts *opts;
> +    int sock = -1;
> +
> +    g_assert(callback != NULL);
> +
> +    opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
> +    qemu_opt_set(opts, "path", path);
> +    sock = unix_connect_opts(opts, errp, callback, opaque);
>      qemu_opts_del(opts);
>      return sock;
>  }
> diff --git a/qemu_socket.h b/qemu_socket.h
> index ff979b5..89a5feb 100644
> --- a/qemu_socket.h
> +++ b/qemu_socket.h
> @@ -58,8 +58,12 @@ const char *inet_strfamily(int family);
>  
>  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_opts(QemuOpts *opts, Error **errp,
> +                      NonBlockingConnectHandler *callback, void *opaque);
>  int unix_connect(const char *path, Error **errp);
> +int unix_nonblocking_connect(const char *str,
> +                             NonBlockingConnectHandler *callback,
> +                             void *opaque, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
>  int parse_host_port(struct sockaddr_in *saddr, const char *str);

Re: [Qemu-devel] [PATCH 05/18] migration: avoid using error_is_set

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:52 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The patch's description is a bit misleading. The real problem this fixes
is that the migration code is using errp to detect "internal" errors,
this means that it relies on errp being non-NULL.

There's no problem using error_set(). Actually, I do prefer using it, because
at least in theory we shouldn't assume the Error object to have this
particular semantic.

One more comment below.

> ---
>  migration-tcp.c |  8 +++++---
>  migration.c     | 13 +++++++------
>  2 file modificati, 12 inserzioni(+), 9 rimozioni(-)
> 
> diff --git a/migration-tcp.c b/migration-tcp.c
> index a15c2b8..78337a3 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -71,14 +71,16 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>  int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>                                   Error **errp)
>  {
> +    Error *local_err = NULL;
> +
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
> -                                     errp);
> -    if (error_is_set(errp)) {
> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
> +    if (local_err != NULL) {
>          migrate_fd_error(s);
> +        error_propagate(errp, local_err);
>          return -1;
>      }
>  
> diff --git a/migration.c b/migration.c
> index 22a05c4..8a04174 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -481,6 +481,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>                   bool has_inc, bool inc, bool has_detach, bool detach,
>                   Error **errp)
>  {
> +    Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      MigrationParams params;
>      const char *p;
> @@ -506,7 +507,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
> -        ret = tcp_start_outgoing_migration(s, p, errp);
> +        ret = tcp_start_outgoing_migration(s, p, &local_err);
>  #if !defined(WIN32)
>      } else if (strstart(uri, "exec:", &p)) {
>          ret = exec_start_outgoing_migration(s, p);
> @@ -520,11 +521,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>          return;
>      }
>  
> -    if (ret < 0) {
> -        if (!error_is_set(errp)) {
> -            DPRINTF("migration failed: %s\n", strerror(-ret));
> -            /* FIXME: we should return meaningful errors */
> -            error_set(errp, QERR_UNDEFINED_ERROR);
> +    if (ret < 0 || local_err) {
> +        if (!local_err) {
> +            error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);

Two problems here. First, ret usually is not -errno. If we really want to
use it here (I think this is great improvement) than we have to fix the
functions called by qmp_migrate() first.

The other problem is just this will produce a weird error message, it's
better to do s/QERR_UNDEFINED_ERROR/"migration has failed".

> +        } else {
> +            error_propagate(errp, local_err);
>          }
>          return;
>      }

Re: [Qemu-devel] [PATCH 06/18] migration: centralize call to migrate_fd_error()

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:53 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> The call to migrate_fd_error() was missing for non-socket backends, so
> centralize it in qmp_migrate().
> 
> Before:
> 
>     (qemu) migrate fd:ffff
>     migrate: An undefined error has occurred
>     (qemu) info migrate
>     (qemu)
> 
> After:
> 
>     (qemu) migrate fd:ffff
>     migrate: An undefined error has occurred
>     (qemu) info migrate
>     capabilities: xbzrle: off
>     Migration status: failed
>     total time: 0 milliseconds
> 
> (The awful error message will be fixed later in the series).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  migration-tcp.c  | 1 -
>  migration-unix.c | 1 -
>  migration.c      | 1 +
>  3 file modificati, 1 inserzione(+), 2 rimozioni(-)
> 
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 78337a3..e8bc76a 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -79,7 +79,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
>  
>      s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
>      if (local_err != NULL) {
> -        migrate_fd_error(s);
>          error_propagate(errp, local_err);
>          return -1;
>      }
> diff --git a/migration-unix.c b/migration-unix.c
> index 169de88..d349662 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -111,7 +111,6 @@ int unix_start_outgoing_migration(MigrationState *s, const char *path)
>  
>      if (ret < 0) {
>          DPRINTF("connect failed\n");
> -        migrate_fd_error(s);
>          return ret;
>      }
>      migrate_fd_connect(s);
> diff --git a/migration.c b/migration.c
> index 8a04174..a56358e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -522,6 +522,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      }
>  
>      if (ret < 0 || local_err) {
> +        migrate_fd_error(s);
>          if (!local_err) {
>              error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
>          } else {

Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:55 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Error propagation is already there for socket backends, but it
> is (and remains) incomplete because no Error is passed to the
> NonBlockingConnectHandler.
> 
> With all protocols understanding Error, the code can be simplified
> by removing the return value.
> 
> Before:
> 
>     (qemu) migrate fd:ffff
>     migrate: An undefined error has occurred
>     (qemu) info migrate
>     (qemu)
> 
> After:
> 
>     (qemu) migrate fd:ffff
>     migrate: File descriptor named 'ffff' has not been found
>     (qemu) info migrate
>     capabilities: xbzrle: off
>     Migration status: failed
>     total time: 0 milliseconds

This is really good, we're missing having good errors in the migrate
command for ages!

But I have comments :)

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration-exec.c |  8 +++-----
>  migration-fd.c   | 11 ++++-------
>  migration-tcp.c  | 13 ++-----------
>  migration-unix.c | 11 ++---------
>  migration.c      | 17 ++++++-----------
>  migration.h      |  9 ++++-----
>  6 file modificati, 21 inserzioni(+), 48 rimozioni(-)
> 
> diff --git a/migration-exec.c b/migration-exec.c
> index 6c97db9..f3baf85 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -60,19 +60,17 @@ static int exec_close(MigrationState *s)
>      return ret;
>  }
>  
> -int exec_start_outgoing_migration(MigrationState *s, const char *command)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>  {
>      FILE *f;
>  
>      f = popen(command, "w");
>      if (f == NULL) {
> -        DPRINTF("Unable to popen exec target\n");

That DPRINTF() usage is really bizarre, it seems its purpose is to report
an error to the user, but that's a debugging call.

I'd let it there and replace it later with proper tracing code, but that's
quite minor for me. Please, at least mention you're dropping it in the log.

>          goto err_after_popen;
>      }
>  
>      s->fd = fileno(f);
>      if (s->fd == -1) {
> -        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
>          goto err_after_open;
>      }
>  
> @@ -85,12 +83,12 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
>      s->write = file_write;
>  
>      migrate_fd_connect(s);
> -    return 0;
> +    return;
>  
>  err_after_open:
>      pclose(f);
>  err_after_popen:
> -    return -1;
> +    error_setg_errno(errp, errno, "failed to popen the migration target");

The pclose() call will override errno.

Otherwise looks good.

>  }
>  
>  static void exec_accept_incoming_migration(void *opaque)
> diff --git a/migration-fd.c b/migration-fd.c
> index 7335167..938a109 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -73,12 +73,11 @@ static int fd_close(MigrationState *s)
>      return 0;
>  }
>  
> -int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
> +void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>  {
> -    s->fd = monitor_get_fd(cur_mon, fdname, NULL);
> +    s->fd = monitor_get_fd(cur_mon, fdname, errp);
>      if (s->fd == -1) {
> -        DPRINTF("fd_migration: invalid file descriptor identifier\n");
> -        goto err_after_get_fd;
> +        return;
>      }
>  
>      if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {

You should set errp here too.

> @@ -91,12 +90,10 @@ int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
>      s->close = fd_close;
>  
>      migrate_fd_connect(s);
> -    return 0;
> +    return;
>  
>  err_after_open:
>      close(s->fd);
> -err_after_get_fd:
> -    return -1;
>  }
>  
>  static void fd_accept_incoming_migration(void *opaque)
> diff --git a/migration-tcp.c b/migration-tcp.c
> index e8bc76a..5e54e68 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -68,22 +68,13 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>      }
>  }
>  
> -int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
> -                                 Error **errp)
> +void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
>      s->get_error = socket_errno;
>      s->write = socket_write;
>      s->close = tcp_close;
>  
> -    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return -1;
> -    }
> -
> -    return 0;
> +    s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp);
>  }
>  
>  static void tcp_accept_incoming_migration(void *opaque)
> diff --git a/migration-unix.c b/migration-unix.c
> index 5387c21..34a413a 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -68,20 +68,13 @@ static void unix_wait_for_connect(int fd, void *opaque)
>      }
>  }
>  
> -int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
> +void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
>      s->get_error = unix_errno;
>      s->write = unix_write;
>      s->close = unix_close;
>  
> -    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> -        return -1;
> -    }
> -    return 0;
> +    s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, errp);
>  }
>  
>  static void unix_accept_incoming_migration(void *opaque)
> diff --git a/migration.c b/migration.c
> index 767e297..f7f0138 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -485,7 +485,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      MigrationState *s = migrate_get_current();
>      MigrationParams params;
>      const char *p;
> -    int ret;
>  
>      params.blk = blk;
>      params.shared = inc;
> @@ -507,27 +506,23 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
> -        ret = tcp_start_outgoing_migration(s, p, &local_err);
> +        tcp_start_outgoing_migration(s, p, &local_err);
>  #if !defined(WIN32)
>      } else if (strstart(uri, "exec:", &p)) {
> -        ret = exec_start_outgoing_migration(s, p);
> +        exec_start_outgoing_migration(s, p, &local_err);
>      } else if (strstart(uri, "unix:", &p)) {
> -        ret = unix_start_outgoing_migration(s, p, &local_err);
> +        unix_start_outgoing_migration(s, p, &local_err);
>      } else if (strstart(uri, "fd:", &p)) {
> -        ret = fd_start_outgoing_migration(s, p);
> +        fd_start_outgoing_migration(s, p, &local_err);
>  #endif
>      } else {
>          error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
>          return;
>      }
>  
> -    if (ret < 0 || local_err) {
> +    if (local_err) {
>          migrate_fd_error(s);
> -        if (!local_err) {
> -            error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
> -        } else {
> -            error_propagate(errp, local_err);
> -        }
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> diff --git a/migration.h b/migration.h
> index e0612a3..275d2d8 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -56,20 +56,19 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);
>  
>  int exec_start_incoming_migration(const char *host_port);
>  
> -int exec_start_outgoing_migration(MigrationState *s, const char *host_port);
> +void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
>  
>  int tcp_start_incoming_migration(const char *host_port, Error **errp);
>  
> -int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
> -                                 Error **errp);
> +void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
>  
>  int unix_start_incoming_migration(const char *path, Error **errp);
>  
> -int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
> +void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
>  
>  int fd_start_incoming_migration(const char *path);
>  
> -int fd_start_outgoing_migration(MigrationState *s, const char *fdname);
> +void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
>  
>  void migrate_fd_error(MigrationState *s);
>  

Re: [Qemu-devel] [PATCH 09/18] migration (incoming): add error propagation for fd and exec protocols

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:56 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> And remove the superfluous integer return value.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  migration-exec.c |  8 +++-----
>  migration-fd.c   |  8 +++-----
>  migration-tcp.c  |  7 ++-----
>  migration-unix.c |  5 ++---
>  migration.c      | 15 ++++++---------
>  migration.h      | 10 +++++-----
>  vl.c             | 16 ++++++----------
>  7 file modificati, 27 inserzioni(+), 42 rimozioni(-)
> 
> diff --git a/migration-exec.c b/migration-exec.c
> index f3baf85..232be6b 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -100,19 +100,17 @@ static void exec_accept_incoming_migration(void *opaque)
>      qemu_fclose(f);
>  }
>  
> -int exec_start_incoming_migration(const char *command)
> +void exec_start_incoming_migration(const char *command, Error **errp)
>  {
>      QEMUFile *f;
>  
>      DPRINTF("Attempting to start an incoming migration\n");
>      f = qemu_popen_cmd(command, "r");
>      if(f == NULL) {
> -        DPRINTF("Unable to apply qemu wrapper to popen file\n");
> -        return -errno;
> +        error_setg_errno(errp, errno, "failed to popen the migration source");
> +        return;
>      }
>  
>      qemu_set_fd_handler2(qemu_stdio_fd(f), NULL,
>  			 exec_accept_incoming_migration, NULL, f);
> -
> -    return 0;
>  }
> diff --git a/migration-fd.c b/migration-fd.c
> index 938a109..24eedd5 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -105,7 +105,7 @@ static void fd_accept_incoming_migration(void *opaque)
>      qemu_fclose(f);
>  }
>  
> -int fd_start_incoming_migration(const char *infd)
> +void fd_start_incoming_migration(const char *infd, Error **errp)
>  {
>      int fd;
>      QEMUFile *f;
> @@ -115,11 +115,9 @@ int fd_start_incoming_migration(const char *infd)
>      fd = strtol(infd, NULL, 0);
>      f = qemu_fdopen(fd, "rb");
>      if(f == NULL) {
> -        DPRINTF("Unable to apply qemu wrapper to file descriptor\n");
> -        return -errno;
> +        error_setg_errno(errp, errno, "failed to open the source descriptor");
> +        return;
>      }
>  
>      qemu_set_fd_handler2(fd, NULL, fd_accept_incoming_migration, NULL, f);
> -
> -    return 0;
>  }
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 5e54e68..46f6ac5 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -111,18 +111,15 @@ out2:
>      close(s);
>  }
>  
> -int tcp_start_incoming_migration(const char *host_port, Error **errp)
> +void tcp_start_incoming_migration(const char *host_port, Error **errp)
>  {
>      int s;
>  
>      s = inet_listen(host_port, NULL, 256, SOCK_STREAM, 0, errp);
> -
>      if (s < 0) {
> -        return -1;
> +        return;
>      }
>  
>      qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
>                           (void *)(intptr_t)s);
> -
> -    return 0;
>  }
> diff --git a/migration-unix.c b/migration-unix.c
> index 34a413a..ed3db3a 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -111,16 +111,15 @@ out2:
>      close(s);
>  }
>  
> -int unix_start_incoming_migration(const char *path, Error **errp)
> +void unix_start_incoming_migration(const char *path, Error **errp)
>  {
>      int s;
>  
>      s = unix_listen(path, NULL, 0, errp);
>      if (s < 0) {
> -        return -1;
> +        return;
>      }
>  
>      qemu_set_fd_handler2(s, NULL, unix_accept_incoming_migration, NULL,
>                           (void *)(intptr_t)s);
> -    return 0;
>  }
> diff --git a/migration.c b/migration.c
> index f7f0138..2802918 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -64,26 +64,23 @@ static MigrationState *migrate_get_current(void)
>      return &current_migration;
>  }
>  
> -int qemu_start_incoming_migration(const char *uri, Error **errp)
> +void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
> -    int ret;
>  
>      if (strstart(uri, "tcp:", &p))
> -        ret = tcp_start_incoming_migration(p, errp);
> +        tcp_start_incoming_migration(p, errp);
>  #if !defined(WIN32)
>      else if (strstart(uri, "exec:", &p))
> -        ret =  exec_start_incoming_migration(p);
> +        exec_start_incoming_migration(p, errp);
>      else if (strstart(uri, "unix:", &p))
> -        ret = unix_start_incoming_migration(p, errp);
> +        unix_start_incoming_migration(p, errp);
>      else if (strstart(uri, "fd:", &p))
> -        ret = fd_start_incoming_migration(p);
> +        fd_start_incoming_migration(p, errp);
>  #endif
>      else {
> -        fprintf(stderr, "unknown migration protocol: %s\n", uri);
> -        ret = -EPROTONOSUPPORT;
> +        error_setg(errp, "unknown migration protocol: %s\n", uri);
>      }
> -    return ret;
>  }
>  
>  void process_incoming_migration(QEMUFile *f)
> diff --git a/migration.h b/migration.h
> index 275d2d8..aa44e1b 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -46,7 +46,7 @@ struct MigrationState
>  
>  void process_incoming_migration(QEMUFile *f);
>  
> -int qemu_start_incoming_migration(const char *uri, Error **errp);
> +void qemu_start_incoming_migration(const char *uri, Error **errp);
>  
>  uint64_t migrate_max_downtime(void);
>  
> @@ -54,19 +54,19 @@ void do_info_migrate_print(Monitor *mon, const QObject *data);
>  
>  void do_info_migrate(Monitor *mon, QObject **ret_data);
>  
> -int exec_start_incoming_migration(const char *host_port);
> +void exec_start_incoming_migration(const char *host_port, Error **errp);
>  
>  void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
>  
> -int tcp_start_incoming_migration(const char *host_port, Error **errp);
> +void tcp_start_incoming_migration(const char *host_port, Error **errp);
>  
>  void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
>  
> -int unix_start_incoming_migration(const char *path, Error **errp);
> +void unix_start_incoming_migration(const char *path, Error **errp);
>  
>  void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
>  
> -int fd_start_incoming_migration(const char *path);
> +void fd_start_incoming_migration(const char *path, Error **errp);
>  
>  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
>  
> diff --git a/vl.c b/vl.c
> index 8d305ca..53917c9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3747,16 +3747,12 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (incoming) {
> -        Error *errp = NULL;
> -        int ret = qemu_start_incoming_migration(incoming, &errp);
> -        if (ret < 0) {
> -            if (error_is_set(&errp)) {
> -                fprintf(stderr, "Migrate: %s\n", error_get_pretty(errp));
> -                error_free(errp);
> -            }
> -            fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
> -                    incoming, ret);
> -            exit(ret);
> +        Error *local_err = NULL;
> +        qemu_start_incoming_migration(incoming, &local_err);
> +        if (local_err) {
> +            fprintf(stderr, "-incoming %s: %s\n", incoming, error_get_pretty(local_err));
> +            error_free(local_err);
> +            exit(1);
>          }
>      } else if (autostart) {
>          vm_start();

Re: [Qemu-devel] [PATCH 10/18] qemu-char: ask and print error information from qemu-sockets

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:57 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Before:
> 
>     $ qemu-system-x86_64 -monitor tcp:localhost:6000
>     (starts despite error)
> 
>     $ qemu-system-x86_64 -monitor tcp:foo.bar:12345
>     getaddrinfo(foo.bar,12345): Name or service not known
>     chardev: opening backend "socket" failed
> 
>     $ qemu-system-x86_64 -monitor tcp:localhost:443,server=on
>     inet_listen_opts: bind(ipv4,127.0.0.1,443): Permission denied
>     inet_listen_opts: FAILED
>     chardev: opening backend "socket" failed
> 
> After:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:localhost:6000
>     x86_64-softmmu/qemu-system-x86_64: -monitor tcp:localhost:6000: Failed to connect to socket: Connection refused
>     chardev: opening backend "socket" failed
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:foo.bar:12345
>     qemu-system-x86_64: -monitor tcp:foo.bar:12345: address resolution failed for foo.bar:12345: Name or service not known
>     chardev: opening backend "socket" failed
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:localhost:443,server=on
>     qemu-system-x86_64: -monitor tcp:localhost:443,server=on: Failed to bind socket: Permission denied
>     chardev: opening backend "socket" failed
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-char.c | 23 +++++++++++++++++------
>  1 file modificato, 17 inserzioni(+), 6 rimozioni(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 8ebd582..04b5c23 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2097,12 +2097,13 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>  {
>      CharDriverState *chr = NULL;
>      NetCharDriver *s = NULL;
> +    Error *local_err = NULL;
>      int fd = -1;
>  
>      chr = g_malloc0(sizeof(CharDriverState));
>      s = g_malloc0(sizeof(NetCharDriver));
>  
> -    fd = inet_dgram_opts(opts, NULL);
> +    fd = inet_dgram_opts(opts, &local_err);
>      if (fd < 0) {
>          fprintf(stderr, "inet_dgram_opts failed\n");
>          goto return_err;
> @@ -2118,6 +2119,10 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>      return chr;
>  
>  return_err:
> +    if (local_err) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    }
>      g_free(chr);
>      g_free(s);
>      if (fd >= 0) {
> @@ -2428,6 +2433,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>  {
>      CharDriverState *chr = NULL;
>      TCPCharDriver *s = NULL;
> +    Error *local_err = NULL;
>      int fd = -1;
>      int is_listen;
>      int is_waitconnect;
> @@ -2448,15 +2454,15 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>  
>      if (is_unix) {
>          if (is_listen) {
> -            fd = unix_listen_opts(opts, NULL);
> +            fd = unix_listen_opts(opts, &local_err);
>          } else {
> -            fd = unix_connect_opts(opts, NULL, NULL, NULL);
> +            fd = unix_connect_opts(opts, &local_err, NULL, NULL);
>          }
>      } else {
>          if (is_listen) {
> -            fd = inet_listen_opts(opts, 0, NULL);
> +            fd = inet_listen_opts(opts, 0, &local_err);
>          } else {
> -            fd = inet_connect_opts(opts, NULL, NULL, NULL);
> +            fd = inet_connect_opts(opts, &local_err, NULL, NULL);
>          }
>      }
>      if (fd < 0) {
> @@ -2517,8 +2523,13 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>      return chr;
>  
>   fail:
> -    if (fd >= 0)
> +    if (local_err) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    }
> +    if (fd >= 0) {
>          closesocket(fd);
> +    }
>      g_free(s);
>      g_free(chr);
>      return NULL;

Re: [Qemu-devel] [PATCH 11/18] nbd: ask and print error information from qemu-sockets

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:58 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Before:
> 
>     $ qemu-system-x86_64 nbd:localhost:12345
>     inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
>     qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
> 
> After:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 nbd:localhost:12345
>     qemu-system-x86_64: Failed to connect to socket: Connection refused
>     qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  nbd.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file modificato, 31 inserzioni(+), 8 rimozioni(-)
> 
> diff --git a/nbd.c b/nbd.c
> index f61a288..cec5a94 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -208,7 +208,14 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
>  
>  int tcp_socket_outgoing_spec(const char *address_and_port)
>  {
> -    return inet_connect(address_and_port, NULL);
> +    Error *local_err = NULL;
> +    int fd = inet_connect(address_and_port, &local_err);
> +
> +    if (local_err != NULL) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);

Can't you propagate errp instead of using qerror_report_err()? This function
should only be used when the caller expects QError semantics.

> +    }
> +    return fd;
>  }
>  
>  int tcp_socket_incoming(const char *address, uint16_t port)
> @@ -220,22 +227,38 @@ int tcp_socket_incoming(const char *address, uint16_t port)
>  
>  int tcp_socket_incoming_spec(const char *address_and_port)
>  {
> -    char *ostr  = NULL;
> -    int olen = 0;
> -    return inet_listen(address_and_port, ostr, olen, SOCK_STREAM, 0, NULL);
> +    Error *local_err = NULL;
> +    int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
> +
> +    if (local_err != NULL) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    }
> +    return fd;
>  }
>  
>  int unix_socket_incoming(const char *path)
>  {
> -    char *ostr = NULL;
> -    int olen = 0;
> +    Error *local_err = NULL;
> +    int fd = unix_listen(path, NULL, 0, &local_err);
>  
> -    return unix_listen(path, ostr, olen, NULL);
> +    if (local_err != NULL) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    }
> +    return fd;
>  }
>  
>  int unix_socket_outgoing(const char *path)
>  {
> -    return unix_connect(path, NULL);
> +    Error *local_err = NULL;
> +    int fd = unix_connect(path, &local_err);
> +
> +    if (local_err != NULL) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +    }
> +    return fd;
>  }
>  
>  /* Basic flow for negotiation

Re: [Qemu-devel] [PATCH 12/18] qemu-ga: ask and print error information from qemu-sockets

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:36:59 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qga/channel-posix.c | 8 +++++---
>  1 file modificato, 5 inserzioni(+), 3 rimozioni(-)
> 
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index e22eee6..d152827 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -181,9 +181,11 @@ 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), NULL);
> -        if (fd == -1) {
> -            g_critical("error opening path: %s", strerror(errno));
> +        Error *local_err = NULL;
> +        int fd = unix_listen(path, NULL, strlen(path), &local_err);
> +        if (local_err != NULL) {
> +            g_critical("%s", error_get_pretty(local_err));
> +            error_free(local_err);
>              return false;
>          }
>          ga_channel_listen_add(c, fd, true);

Re: [Qemu-devel] [PATCH 13/18] vnc: add error propagation to vnc_display_open

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:37:00 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Before:
> 
>     $ qemu-system-x86_64 -vnc foo.bar:12345
>     getaddrinfo(foo.bar,18245): Name or service not known
>     Failed to start VNC server on `foo.bar:12345'
> 
>     $ qemu-system-x86_64 -vnc localhost:12345,reverse=on
>     inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
>     Failed to start VNC server on `localhost:12345,reverse=on'
> 
> After:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -vnc foo.bar:12345
>     qemu-system-x86_64: address resolution failed for foo.bar:18245: Name or service not known
>     Failed to start VNC server on `foo.bar:12345'
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -vnc localhost:12345,reverse=on
>     qemu-system-x86_64: Failed to connect to socket: Connection refused
>     Failed to start VNC server on `localhost:12345,reverse=on'
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  console.h |  2 +-
>  qmp.c     |  6 ++----
>  ui/vnc.c  | 67 +++++++++++++++++++++++++++++----------------------------------
>  vl.c      |  5 ++++-
>  4 file modificati, 38 inserzioni(+), 42 rimozioni(-)
> 
> diff --git a/console.h b/console.h
> index f990684..fcf2fc5 100644
> --- a/console.h
> +++ b/console.h
> @@ -378,7 +378,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
>  /* vnc.c */
>  void vnc_display_init(DisplayState *ds);
>  void vnc_display_close(DisplayState *ds);
> -int vnc_display_open(DisplayState *ds, const char *display);
> +int vnc_display_open(DisplayState *ds, const char *display, Error **errp);
>  void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
>  int vnc_display_disable_login(DisplayState *ds);
>  char *vnc_display_local_addr(DisplayState *ds);
> diff --git a/qmp.c b/qmp.c
> index 36c54c5..31bc3bf 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -349,11 +349,9 @@ void qmp_change_vnc_password(const char *password, Error **errp)
>      }
>  }
>  
> -static void qmp_change_vnc_listen(const char *target, Error **err)
> +static void qmp_change_vnc_listen(const char *target, Error **errp)
>  {
> -    if (vnc_display_open(NULL, target) < 0) {
> -        error_set(err, QERR_VNC_SERVER_FAILED, target);
> -    }
> +    vnc_display_open(NULL, target, errp);
>  }
>  
>  static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 235596e..0d92670 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2844,7 +2844,7 @@ char *vnc_display_local_addr(DisplayState *ds)
>      return vnc_socket_local_addr("%s:%s", vs->lsock);
>  }
>  
> -int vnc_display_open(DisplayState *ds, const char *display)
> +int vnc_display_open(DisplayState *ds, const char *display, Error **errp)

Can't return void?

>  {
>      VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
>      const char *options;
> @@ -2863,13 +2863,12 @@ int vnc_display_open(DisplayState *ds, const char *display)
>      int lock_key_sync = 1;
>  
>      if (!vnc_display)
> -        return -1;
> +        goto fail;
>      vnc_display_close(ds);
>      if (strcmp(display, "none") == 0)
>          return 0;
>  
> -    if (!(vs->display = strdup(display)))
> -        return -1;
> +    vs->display = g_strdup(display);
>      vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>  
>      options = display;
> @@ -2877,13 +2876,11 @@ int vnc_display_open(DisplayState *ds, const char *display)
>          options++;
>          if (strncmp(options, "password", 8) == 0) {
>              if (fips_get_state()) {
> -                fprintf(stderr,
> -                        "VNC password auth disabled due to FIPS mode, "
> -                        "consider using the VeNCrypt or SASL authentication "
> -                        "methods as an alternative\n");
> -                g_free(vs->display);
> -                vs->display = NULL;
> -                return -1;
> +                error_setg(errp,
> +                           "VNC password auth disabled due to FIPS mode, "
> +                           "consider using the VeNCrypt or SASL authentication "
> +                           "methods as an alternative\n");
> +                goto fail;
>              }
>              password = 1; /* Require password auth */
>          } else if (strncmp(options, "reverse", 7) == 0) {
> @@ -2913,18 +2910,14 @@ int vnc_display_open(DisplayState *ds, const char *display)
>  
>                  VNC_DEBUG("Trying certificate path '%s'\n", path);
>                  if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
> -                    fprintf(stderr, "Failed to find x509 certificates/keys in %s\n", path);
> +                    error_setg(errp, "Failed to find x509 certificates/keys in %s\n", path);
>                      g_free(path);
> -                    g_free(vs->display);
> -                    vs->display = NULL;
> -                    return -1;
> +                    goto fail;
>                  }
>                  g_free(path);
>              } else {
> -                fprintf(stderr, "No certificate path provided\n");
> -                g_free(vs->display);
> -                vs->display = NULL;
> -                return -1;
> +                error_setg(errp, "No certificate path provided\n");
> +                goto fail;
>              }
>  #endif
>  #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
> @@ -2943,10 +2936,8 @@ int vnc_display_open(DisplayState *ds, const char *display)
>              } else if (strncmp(options+6, "force-shared", 12) == 0) {
>                  vs->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
>              } else {
> -                fprintf(stderr, "unknown vnc share= option\n");
> -                g_free(vs->display);
> -                vs->display = NULL;
> -                return -1;
> +                error_setg(errp, "unknown vnc share= option\n");
> +                goto fail;
>              }
>          }
>      }
> @@ -3047,11 +3038,9 @@ int vnc_display_open(DisplayState *ds, const char *display)
>  
>  #ifdef CONFIG_VNC_SASL
>      if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
> -        fprintf(stderr, "Failed to initialize SASL auth %s",
> -                sasl_errstring(saslErr, NULL, NULL));
> -        g_free(vs->display);
> -        vs->display = NULL;
> -        return -1;
> +        error_setg(errp, "Failed to initialize SASL auth %s",
> +                   sasl_errstring(saslErr, NULL, NULL));
> +        goto fail;
>      }
>  #endif
>      vs->lock_key_sync = lock_key_sync;
> @@ -3059,13 +3048,11 @@ 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, NULL);
> +            vs->lsock = unix_connect(display+5, errp);
>          else
> -            vs->lsock = inet_connect(display, NULL);
> +            vs->lsock = inet_connect(display, errp);
>          if (-1 == vs->lsock) {
> -            g_free(vs->display);
> -            vs->display = NULL;
> -            return -1;
> +            goto fail;
>          } else {
>              int csock = vs->lsock;
>              vs->lsock = -1;
> @@ -3079,20 +3066,28 @@ 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, NULL);
> +            vs->lsock = unix_listen(display+5, dpy+5, 256-5, errp);
>          } else {
>              vs->lsock = inet_listen(display, dpy, 256,
> -                                    SOCK_STREAM, 5900, NULL);
> +                                    SOCK_STREAM, 5900, errp);
>          }
>          if (-1 == vs->lsock) {
>              g_free(dpy);
> -            return -1;
> +            goto fail;
>          } else {
>              g_free(vs->display);
>              vs->display = dpy;
>          }
>      }
>      return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
> +
> +fail:
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_VNC_SERVER_FAILED, display);

Please, use error_setg() instead. QERR_ macros are deprecated.

> +    }
> +    g_free(vs->display);
> +    vs->display = NULL;
> +    return -1;
>  }
>  
>  void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
> diff --git a/vl.c b/vl.c
> index 53917c9..45a5ba5 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3692,8 +3692,11 @@ int main(int argc, char **argv, char **envp)
>  #ifdef CONFIG_VNC
>      /* init remote displays */
>      if (vnc_display) {
> +        Error *local_err = NULL;
>          vnc_display_init(ds);
> -        if (vnc_display_open(ds, vnc_display) < 0) {
> +        if (vnc_display_open(ds, vnc_display, &local_err) < 0) {
> +            qerror_report_err(local_err);
> +            error_free(local_err);
>              fprintf(stderr, "Failed to start VNC server on `%s'\n",
>                      vnc_display);
>              exit(1);

Why do you need to call qerror_report_err()? I'd just do:

fprintf(stderr, "Failed to start VNC server on display '%s': %s\n",
                vnc_display, error_get_pretty(local_err));

Re: [Qemu-devel] [PATCH 05/18] migration: avoid using error_is_set

[Paolo Bonzini]

Il 04/10/2012 20:06, Luiz Capitulino ha scritto:
>> > +    if (ret < 0 || local_err) {
>> > +        if (!local_err) {
>> > +            error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
> Two problems here. First, ret usually is not -errno. If we really want to
> use it here (I think this is great improvement) than we have to fix the
> functions called by qmp_migrate() first.

Yes, it is only -errno for migration-unix.c, but anyway...

> The other problem is just this will produce a weird error message, it's
> better to do s/QERR_UNDEFINED_ERROR/"migration has failed".
> 

... both problems are fixed later in the series when ret is eliminated.

Paolo

Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

[Paolo Bonzini]

Il 04/10/2012 20:24, Luiz Capitulino ha scritto:
> That DPRINTF() usage is really bizarre, it seems its purpose is to report
> an error to the user, but that's a debugging call.
> 
> I'd let it there and replace it later with proper tracing code, but that's
> quite minor for me. Please, at least mention you're dropping it in the log.

This one is not dropped, it becomes the reported error message.

>> >          goto err_after_popen;
>> >      }
>> >  
>> >      s->fd = fileno(f);
>> >      if (s->fd == -1) {
>> > -        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");

This one is dropped, but I wanted to delete the check altogether.
fileno() should only fail if it detects somehow that its argument is not
a valid stream, which is obviously not the case.

Would that be better?  It would also fix the clobbering of errno.

>> >          goto err_after_open;
>> >      }
>> >  
>> > @@ -85,12 +83,12 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
>> >      s->write = file_write;
>> >  
>> >      migrate_fd_connect(s);
>> > -    return 0;
>> > +    return;
>> >  
>> >  err_after_open:
>> >      pclose(f);
>> >  err_after_popen:
>> > -    return -1;
>> > +    error_setg_errno(errp, errno, "failed to popen the migration target");
> The pclose() call will override errno.

Paolo

Re: [Qemu-devel] [PATCH 11/18] nbd: ask and print error information from qemu-sockets

[Paolo Bonzini]

Il 04/10/2012 22:08, Luiz Capitulino ha scritto:
> On Wed,  3 Oct 2012 16:36:58 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Before:
>>
>>     $ qemu-system-x86_64 nbd:localhost:12345
>>     inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
>>     qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
>>
>> After:
>>
>>     $ x86_64-softmmu/qemu-system-x86_64 nbd:localhost:12345
>>     qemu-system-x86_64: Failed to connect to socket: Connection refused
>>     qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  nbd.c | 39 +++++++++++++++++++++++++++++++--------
>>  1 file modificato, 31 inserzioni(+), 8 rimozioni(-)
>>
>> diff --git a/nbd.c b/nbd.c
>> index f61a288..cec5a94 100644
>> --- a/nbd.c
>> +++ b/nbd.c
>> @@ -208,7 +208,14 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
>>  
>>  int tcp_socket_outgoing_spec(const char *address_and_port)
>>  {
>> -    return inet_connect(address_and_port, NULL);
>> +    Error *local_err = NULL;
>> +    int fd = inet_connect(address_and_port, &local_err);
>> +
>> +    if (local_err != NULL) {
>> +        qerror_report_err(local_err);
>> +        error_free(local_err);
> 
> Can't you propagate errp instead of using qerror_report_err()? This function
> should only be used when the caller expects QError semantics.

No, I cannot.  These functions are used only for a) qemu-nbd, for which
qerror_report_err() is ok; b) the NBD driver's bdrv_open, which is not
able to propagate errors yet.

When error propagation is added to bdrv_open they can just disappear,
replaced by direct calls to functions in qemu-sockets.c.

Paolo

>> +    }
>> +    return fd;
>>  }
>>  
>>  int tcp_socket_incoming(const char *address, uint16_t port)
>> @@ -220,22 +227,38 @@ int tcp_socket_incoming(const char *address, uint16_t port)
>>  
>>  int tcp_socket_incoming_spec(const char *address_and_port)
>>  {
>> -    char *ostr  = NULL;
>> -    int olen = 0;
>> -    return inet_listen(address_and_port, ostr, olen, SOCK_STREAM, 0, NULL);
>> +    Error *local_err = NULL;
>> +    int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
>> +
>> +    if (local_err != NULL) {
>> +        qerror_report_err(local_err);
>> +        error_free(local_err);
>> +    }
>> +    return fd;
>>  }
>>  
>>  int unix_socket_incoming(const char *path)
>>  {
>> -    char *ostr = NULL;
>> -    int olen = 0;
>> +    Error *local_err = NULL;
>> +    int fd = unix_listen(path, NULL, 0, &local_err);
>>  
>> -    return unix_listen(path, ostr, olen, NULL);
>> +    if (local_err != NULL) {
>> +        qerror_report_err(local_err);
>> +        error_free(local_err);
>> +    }
>> +    return fd;
>>  }
>>  
>>  int unix_socket_outgoing(const char *path)
>>  {
>> -    return unix_connect(path, NULL);
>> +    Error *local_err = NULL;
>> +    int fd = unix_connect(path, &local_err);
>> +
>> +    if (local_err != NULL) {
>> +        qerror_report_err(local_err);
>> +        error_free(local_err);
>> +    }
>> +    return fd;
>>  }
>>  
>>  /* Basic flow for negotiation
> 
> 
> 

Re: [Qemu-devel] [PATCH 13/18] vnc: add error propagation to vnc_display_open

[Paolo Bonzini]

Il 04/10/2012 22:29, Luiz Capitulino ha scritto:
> On Wed,  3 Oct 2012 16:37:00 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Before:
>>
>>     $ qemu-system-x86_64 -vnc foo.bar:12345
>>     getaddrinfo(foo.bar,18245): Name or service not known
>>     Failed to start VNC server on `foo.bar:12345'
>>
>>     $ qemu-system-x86_64 -vnc localhost:12345,reverse=on
>>     inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
>>     Failed to start VNC server on `localhost:12345,reverse=on'
>>
>> After:
>>
>>     $ x86_64-softmmu/qemu-system-x86_64 -vnc foo.bar:12345
>>     qemu-system-x86_64: address resolution failed for foo.bar:18245: Name or service not known
>>     Failed to start VNC server on `foo.bar:12345'
>>
>>     $ x86_64-softmmu/qemu-system-x86_64 -vnc localhost:12345,reverse=on
>>     qemu-system-x86_64: Failed to connect to socket: Connection refused
>>     Failed to start VNC server on `localhost:12345,reverse=on'
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  console.h |  2 +-
>>  qmp.c     |  6 ++----
>>  ui/vnc.c  | 67 +++++++++++++++++++++++++++++----------------------------------
>>  vl.c      |  5 ++++-
>>  4 file modificati, 38 inserzioni(+), 42 rimozioni(-)
>>
>> diff --git a/console.h b/console.h
>> index f990684..fcf2fc5 100644
>> --- a/console.h
>> +++ b/console.h
>> @@ -378,7 +378,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
>>  /* vnc.c */
>>  void vnc_display_init(DisplayState *ds);
>>  void vnc_display_close(DisplayState *ds);
>> -int vnc_display_open(DisplayState *ds, const char *display);
>> +int vnc_display_open(DisplayState *ds, const char *display, Error **errp);
>>  void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
>>  int vnc_display_disable_login(DisplayState *ds);
>>  char *vnc_display_local_addr(DisplayState *ds);
>> diff --git a/qmp.c b/qmp.c
>> index 36c54c5..31bc3bf 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -349,11 +349,9 @@ void qmp_change_vnc_password(const char *password, Error **errp)
>>      }
>>  }
>>  
>> -static void qmp_change_vnc_listen(const char *target, Error **err)
>> +static void qmp_change_vnc_listen(const char *target, Error **errp)
>>  {
>> -    if (vnc_display_open(NULL, target) < 0) {
>> -        error_set(err, QERR_VNC_SERVER_FAILED, target);
>> -    }
>> +    vnc_display_open(NULL, target, errp);
>>  }
>>  
>>  static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 235596e..0d92670 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -2844,7 +2844,7 @@ char *vnc_display_local_addr(DisplayState *ds)
>>      return vnc_socket_local_addr("%s:%s", vs->lsock);
>>  }
>>  
>> -int vnc_display_open(DisplayState *ds, const char *display)
>> +int vnc_display_open(DisplayState *ds, const char *display, Error **errp)
> 
> Can't return void?

Ok.

>>  {
>>      VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
>>      const char *options;
>> @@ -2863,13 +2863,12 @@ int vnc_display_open(DisplayState *ds, const char *display)
>>      int lock_key_sync = 1;
>>  
>>      if (!vnc_display)
>> -        return -1;
>> +        goto fail;
>>      vnc_display_close(ds);
>>      if (strcmp(display, "none") == 0)
>>          return 0;
>>  
>> -    if (!(vs->display = strdup(display)))
>> -        return -1;
>> +    vs->display = g_strdup(display);
>>      vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>>  
>>      options = display;
>> @@ -2877,13 +2876,11 @@ int vnc_display_open(DisplayState *ds, const char *display)
>>          options++;
>>          if (strncmp(options, "password", 8) == 0) {
>>              if (fips_get_state()) {
>> -                fprintf(stderr,
>> -                        "VNC password auth disabled due to FIPS mode, "
>> -                        "consider using the VeNCrypt or SASL authentication "
>> -                        "methods as an alternative\n");
>> -                g_free(vs->display);
>> -                vs->display = NULL;
>> -                return -1;
>> +                error_setg(errp,
>> +                           "VNC password auth disabled due to FIPS mode, "
>> +                           "consider using the VeNCrypt or SASL authentication "
>> +                           "methods as an alternative\n");
>> +                goto fail;
>>              }
>>              password = 1; /* Require password auth */
>>          } else if (strncmp(options, "reverse", 7) == 0) {
>> @@ -2913,18 +2910,14 @@ int vnc_display_open(DisplayState *ds, const char *display)
>>  
>>                  VNC_DEBUG("Trying certificate path '%s'\n", path);
>>                  if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
>> -                    fprintf(stderr, "Failed to find x509 certificates/keys in %s\n", path);
>> +                    error_setg(errp, "Failed to find x509 certificates/keys in %s\n", path);
>>                      g_free(path);
>> -                    g_free(vs->display);
>> -                    vs->display = NULL;
>> -                    return -1;
>> +                    goto fail;
>>                  }
>>                  g_free(path);
>>              } else {
>> -                fprintf(stderr, "No certificate path provided\n");
>> -                g_free(vs->display);
>> -                vs->display = NULL;
>> -                return -1;
>> +                error_setg(errp, "No certificate path provided\n");
>> +                goto fail;
>>              }
>>  #endif
>>  #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
>> @@ -2943,10 +2936,8 @@ int vnc_display_open(DisplayState *ds, const char *display)
>>              } else if (strncmp(options+6, "force-shared", 12) == 0) {
>>                  vs->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
>>              } else {
>> -                fprintf(stderr, "unknown vnc share= option\n");
>> -                g_free(vs->display);
>> -                vs->display = NULL;
>> -                return -1;
>> +                error_setg(errp, "unknown vnc share= option\n");
>> +                goto fail;
>>              }
>>          }
>>      }
>> @@ -3047,11 +3038,9 @@ int vnc_display_open(DisplayState *ds, const char *display)
>>  
>>  #ifdef CONFIG_VNC_SASL
>>      if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
>> -        fprintf(stderr, "Failed to initialize SASL auth %s",
>> -                sasl_errstring(saslErr, NULL, NULL));
>> -        g_free(vs->display);
>> -        vs->display = NULL;
>> -        return -1;
>> +        error_setg(errp, "Failed to initialize SASL auth %s",
>> +                   sasl_errstring(saslErr, NULL, NULL));
>> +        goto fail;
>>      }
>>  #endif
>>      vs->lock_key_sync = lock_key_sync;
>> @@ -3059,13 +3048,11 @@ 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, NULL);
>> +            vs->lsock = unix_connect(display+5, errp);
>>          else
>> -            vs->lsock = inet_connect(display, NULL);
>> +            vs->lsock = inet_connect(display, errp);
>>          if (-1 == vs->lsock) {
>> -            g_free(vs->display);
>> -            vs->display = NULL;
>> -            return -1;
>> +            goto fail;
>>          } else {
>>              int csock = vs->lsock;
>>              vs->lsock = -1;
>> @@ -3079,20 +3066,28 @@ 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, NULL);
>> +            vs->lsock = unix_listen(display+5, dpy+5, 256-5, errp);
>>          } else {
>>              vs->lsock = inet_listen(display, dpy, 256,
>> -                                    SOCK_STREAM, 5900, NULL);
>> +                                    SOCK_STREAM, 5900, errp);
>>          }
>>          if (-1 == vs->lsock) {
>>              g_free(dpy);
>> -            return -1;
>> +            goto fail;
>>          } else {
>>              g_free(vs->display);
>>              vs->display = dpy;
>>          }
>>      }
>>      return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
>> +
>> +fail:
>> +    if (!error_is_set(errp)) {
>> +        error_set(errp, QERR_VNC_SERVER_FAILED, display);
> 
> Please, use error_setg() instead. QERR_ macros are deprecated.

I'm not introducing a new one.  One thing at a time, please.

>> +    }
>> +    g_free(vs->display);
>> +    vs->display = NULL;
>> +    return -1;
>>  }
>>  
>>  void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
>> diff --git a/vl.c b/vl.c
>> index 53917c9..45a5ba5 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3692,8 +3692,11 @@ int main(int argc, char **argv, char **envp)
>>  #ifdef CONFIG_VNC
>>      /* init remote displays */
>>      if (vnc_display) {
>> +        Error *local_err = NULL;
>>          vnc_display_init(ds);
>> -        if (vnc_display_open(ds, vnc_display) < 0) {
>> +        if (vnc_display_open(ds, vnc_display, &local_err) < 0) {
>> +            qerror_report_err(local_err);
>> +            error_free(local_err);
>>              fprintf(stderr, "Failed to start VNC server on `%s'\n",
>>                      vnc_display);
>>              exit(1);
> 
> Why do you need to call qerror_report_err()? I'd just do:
> 
> fprintf(stderr, "Failed to start VNC server on display '%s': %s\n",
>                 vnc_display, error_get_pretty(local_err));
> 
> 

Ok.

Re: [Qemu-devel] [PATCH 13/18] vnc: add error propagation to vnc_display_open

[Paolo Bonzini]

Il 05/10/2012 08:28, Paolo Bonzini ha scritto:
>>>  void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
>>> diff --git a/vl.c b/vl.c
>>> index 53917c9..45a5ba5 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -3692,8 +3692,11 @@ int main(int argc, char **argv, char **envp)
>>>  #ifdef CONFIG_VNC
>>>      /* init remote displays */
>>>      if (vnc_display) {
>>> +        Error *local_err = NULL;
>>>          vnc_display_init(ds);
>>> -        if (vnc_display_open(ds, vnc_display) < 0) {
>>> +        if (vnc_display_open(ds, vnc_display, &local_err) < 0) {
>>> +            qerror_report_err(local_err);
>>> +            error_free(local_err);
>>>              fprintf(stderr, "Failed to start VNC server on `%s'\n",
>>>                      vnc_display);
>>>              exit(1);
>>
>> Why do you need to call qerror_report_err()? I'd just do:
>>
>> fprintf(stderr, "Failed to start VNC server on display '%s': %s\n",
>>                 vnc_display, error_get_pretty(local_err));
> 
> Ok.

Hmm, qerror_report_err is more consistent (it prints the program name).

Paolo

Re: [Qemu-devel] [PATCH 04/18] qemu-sockets: add nonblocking connect for Unix sockets

[Paolo Bonzini]

Il 04/10/2012 19:38, Luiz Capitulino ha scritto:
> On Wed,  3 Oct 2012 16:36:51 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> This looks like a bug fix, because if is_waitconnect==false unix_connect_opts()
> shouldn't block. Am I right?

It could be a start, but it doesn't fix it yet.  qemu-char always calls
inet_connect_opts() and unix_connect_opts() in blocking mode.

I wonder if Orit's patches have broken wait=off, need to check.

Paolo

Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

[Luiz Capitulino]

On Fri, 05 Oct 2012 08:25:46 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/10/2012 20:24, Luiz Capitulino ha scritto:
> > That DPRINTF() usage is really bizarre, it seems its purpose is to report
> > an error to the user, but that's a debugging call.
> > 
> > I'd let it there and replace it later with proper tracing code, but that's
> > quite minor for me. Please, at least mention you're dropping it in the log.
> 
> This one is not dropped, it becomes the reported error message.

What I meant is that the error/debug message won't be printed the same way
it was before. This is an improvement, but it's a good idea to mention it.

> >> >          goto err_after_popen;
> >> >      }
> >> >  
> >> >      s->fd = fileno(f);
> >> >      if (s->fd == -1) {
> >> > -        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
> 
> This one is dropped, but I wanted to delete the check altogether.
> fileno() should only fail if it detects somehow that its argument is not
> a valid stream, which is obviously not the case.
> 
> Would that be better?  It would also fix the clobbering of errno.

I guess so. Is it possible for popen() to return success but then set the
FILE's fd to -1? Maybe change to an assert() instead?

Re: [Qemu-devel] [PATCH 11/18] nbd: ask and print error information from qemu-sockets

[Luiz Capitulino]

On Fri, 05 Oct 2012 08:27:25 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/10/2012 22:08, Luiz Capitulino ha scritto:
> > On Wed,  3 Oct 2012 16:36:58 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> Before:
> >>
> >>     $ qemu-system-x86_64 nbd:localhost:12345
> >>     inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
> >>     qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
> >>
> >> After:
> >>
> >>     $ x86_64-softmmu/qemu-system-x86_64 nbd:localhost:12345
> >>     qemu-system-x86_64: Failed to connect to socket: Connection refused
> >>     qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  nbd.c | 39 +++++++++++++++++++++++++++++++--------
> >>  1 file modificato, 31 inserzioni(+), 8 rimozioni(-)
> >>
> >> diff --git a/nbd.c b/nbd.c
> >> index f61a288..cec5a94 100644
> >> --- a/nbd.c
> >> +++ b/nbd.c
> >> @@ -208,7 +208,14 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
> >>  
> >>  int tcp_socket_outgoing_spec(const char *address_and_port)
> >>  {
> >> -    return inet_connect(address_and_port, NULL);
> >> +    Error *local_err = NULL;
> >> +    int fd = inet_connect(address_and_port, &local_err);
> >> +
> >> +    if (local_err != NULL) {
> >> +        qerror_report_err(local_err);
> >> +        error_free(local_err);
> > 
> > Can't you propagate errp instead of using qerror_report_err()? This function
> > should only be used when the caller expects QError semantics.
> 
> No, I cannot.  These functions are used only for a) qemu-nbd, for which
> qerror_report_err() is ok; b) the NBD driver's bdrv_open, which is not
> able to propagate errors yet.
> 
> When error propagation is added to bdrv_open they can just disappear,
> replaced by direct calls to functions in qemu-sockets.c.

Ok. I've started working on adding error propagation to bdrv_open(), should
have a series soon.

Re: [Qemu-devel] [PATCH 08/18] migration (outgoing): add error propagation for fd and exec protocols

[Paolo Bonzini]

Il 05/10/2012 14:41, Luiz Capitulino ha scritto:
>>> > > That DPRINTF() usage is really bizarre, it seems its purpose is to report
>>> > > an error to the user, but that's a debugging call.
>>> > > 
>>> > > I'd let it there and replace it later with proper tracing code, but that's
>>> > > quite minor for me. Please, at least mention you're dropping it in the log.
>> > 
>> > This one is not dropped, it becomes the reported error message.
> What I meant is that the error/debug message won't be printed the same way
> it was before. This is an improvement, but it's a good idea to mention it.
> 

I mentioned it for fd, but the popen() function only returns NULL if the
fork(2) or pipe(2) calls fail, or if it cannot allocate memory, so I
have no idea how to trigger it.

>> It would also fix the clobbering of errno.
> 
> I guess so. Is it possible for popen() to return success but then set the
> FILE's fd to -1?

No.  I will add the assert().

Paolo

Re: [Qemu-devel] [PATCH 14/18] qemu-sockets: include strerror or gai_strerror output in error messages

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:37:01 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Among others, before:
> 
>     $ qemu-system-x86_64 -chardev socket,port=12345,id=char
>     inet_connect: host and/or port not specified
>     chardev: opening backend "socket" failed
> 
> After:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -chardev socket,port=12345,id=char
>     qemu-system-x86_64: -chardev socket,port=12345,id=char: host and/or port not specified
>     chardev: opening backend "socket" failed
> 
> perror and fprintf can be removed because all clients can now
> consume Errors properly.

Will trust you there :)

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I honestly don't like to keep the QERR_ macros around, I think we should
kill them when we have the opportunity to do so, but patch looks good so:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-sockets.c | 30 +++++++++---------------------
>  1 file modificato, 9 inserzioni(+), 21 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 341ae21..8ab2d0c 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -120,8 +120,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
>  
>      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_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_setg(errp, "host and/or port not specified");
>          return -1;
>      }
>      pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port"));
> @@ -138,9 +137,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
>          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_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
> +                   gai_strerror(rc));
>          return -1;
>      }
>  
> @@ -151,10 +149,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
>  		        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));
>              if (!e->ai_next) {
> -                error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +                error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
>              }
>              continue;
>          }
> @@ -176,24 +172,19 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
>                  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));
>                  if (!e->ai_next) {
> -                    error_set(errp, QERR_SOCKET_BIND_FAILED);
> +                    error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
>                  }
>              }
>          }
>          closesocket(slisten);
>      }
> -    fprintf(stderr, "%s: FAILED\n", __FUNCTION__);
>      freeaddrinfo(res);
>      return -1;
>  
>  listen:
>      if (listen(slisten,1) != 0) {
> -        error_set(errp, QERR_SOCKET_LISTEN_FAILED);
> -        perror("listen");
> +        error_set_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
>          closesocket(slisten);
>          freeaddrinfo(res);
>          return -1;
> @@ -325,9 +316,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>      addr = qemu_opt_get(opts, "host");
>      port = qemu_opt_get(opts, "port");
>      if (addr == NULL || port == NULL) {
> -        fprintf(stderr,
> -                "inet_parse_connect_opts: host and/or port not specified\n");
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_setg(errp, "host and/or port not specified");
>          return NULL;
>      }
>  
> @@ -341,9 +330,8 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
>      /* lookup */
>      rc = getaddrinfo(addr, port, &ai, &res);
>      if (rc != 0) {
> -        fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> -        error_set(errp, QERR_SOCKET_CREATE_FAILED);
> +        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
> +                   gai_strerror(rc));
>          return NULL;
>      }
>      return res;

Re: [Qemu-devel] [PATCH 15/18] qemu-sockets: add error propagation to inet_connect_addr

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:37:02 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> perror and fprintf can be removed because all clients can now consume
> Errors properly.  However, we need to change the non-blocking connect
> handlers to take an Error, in order to improve error handling for
> migration with the TCP protocol.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-sockets.c | 15 ++++++---------
>  1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 8ab2d0c..b096f49 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -216,7 +216,7 @@ typedef struct ConnectState {
>  } ConnectState;
>  
>  static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> -                             ConnectState *connect_state);
> +                             ConnectState *connect_state, Error **errp);
>  
>  static void wait_for_connect(void *opaque)
>  {
> @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque)
>      if (s->current_addr) {
>          while (s->current_addr->ai_next != NULL && s->fd < 0) {
>              s->current_addr = s->current_addr->ai_next;
> -            s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> +            s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
>              /* connect in progress */
>              if (in_progress) {
>                  return;
> @@ -264,7 +264,7 @@ static void wait_for_connect(void *opaque)
>  }
>  
>  static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> -                             ConnectState *connect_state)
> +                             ConnectState *connect_state, Error **errp)
>  {
>      int sock, rc;
>  
> @@ -272,8 +272,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
>  
>      sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
>      if (sock < 0) {
> -        fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> -                inet_strfamily(addr->ai_family), strerror(errno));
> +        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>      setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> @@ -294,6 +293,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
>                               connect_state);
>          *in_progress = true;
>      } else if (rc < 0) {
> +        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);

The patch look fine, but as I said in my previous email I really dislike
seeing QERR_ macros usage in new code. If the problem here is to duplicate
the error message, then maybe we could put this connect() block in a wrapper.

>          closesocket(sock);
>          return -1;
>      }
> @@ -376,7 +376,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>          if (connect_state != NULL) {
>              connect_state->current_addr = e;
>          }
> -        sock = inet_connect_addr(e, &in_progress, connect_state);
> +        sock = inet_connect_addr(e, &in_progress, connect_state, errp);
>          if (in_progress) {
>              return sock;
>          } else if (sock >= 0) {
> @@ -387,9 +387,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>              break;
>          }
>      }
> -    if (sock < 0) {
> -        error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> -    }
>      g_free(connect_state);
>      freeaddrinfo(res);
>      return sock;

Re: [Qemu-devel] [PATCH 15/18] qemu-sockets: add error propagation to inet_connect_addr

[Paolo Bonzini]

Il 09/10/2012 16:58, Luiz Capitulino ha scritto:
>> > +        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> The patch look fine, but as I said in my previous email I really dislike
> seeing QERR_ macros usage in new code. If the problem here is to duplicate
> the error message, then maybe we could put this connect() block in a wrapper.

Again: one thing at a time.

The only obvious step is to remove QERR_ constants that are used just
once.  Everything else should be done carefully because if later you
decide to add something to the errors (for example the bind or connect
argument) you have no protection against typos, etc.

Adding new QERR_ constants is somewhat harmful, but using the old ones
absolutely is not.

Paolo

Re: [Qemu-devel] [PATCH 15/18] qemu-sockets: add error propagation to inet_connect_addr

[Luiz Capitulino]

On Tue, 09 Oct 2012 17:02:02 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 09/10/2012 16:58, Luiz Capitulino ha scritto:
> >> > +        error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> > The patch look fine, but as I said in my previous email I really dislike
> > seeing QERR_ macros usage in new code. If the problem here is to duplicate
> > the error message, then maybe we could put this connect() block in a wrapper.
> 
> Again: one thing at a time.

Right.

> The only obvious step is to remove QERR_ constants that are used just
> once. 

I agree.

> Everything else should be done carefully because if later you
> decide to add something to the errors (for example the bind or connect
> argument) you have no protection against typos, etc.

I don't fully agree. The best way to do this is to add new wrappers
to bind() or connect() that take an Error ** argument and not extend
the QERR_ macros. But I agree that we can do this later.

> Adding new QERR_ constants is somewhat harmful, but using the old ones
> absolutely is not.

Yes:

 Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

Re: [Qemu-devel] [PATCH 16/18] qemu-sockets: add error propagation to inet_dgram_opts

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:37:03 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Before:
> 
>     $ qemu-system-x86_64 -monitor udp:localhost:631@localhost:631
>     inet_dgram_opts: bind(ipv4,127.0.0.1,631): OK
>     inet_dgram_opts failed
>     chardev: opening backend "udp" failed
> 
> After:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -monitor udp:localhost:631@localhost:631
>     qemu-system-x86_64: -monitor udp:localhost:631@localhost:631: Failed to bind socket: Address already in use
>     chardev: opening backend "udp" failed
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-char.c    |  1 -
>  qemu-sockets.c | 34 ++++++++--------------------------
>  2 file modificati, 8 inserzioni(+), 27 rimozioni(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 04b5c23..afe2bfb 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2105,7 +2105,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>  
>      fd = inet_dgram_opts(opts, &local_err);
>      if (fd < 0) {
> -        fprintf(stderr, "inet_dgram_opts failed\n");
>          goto return_err;
>      }
>  
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index b096f49..6f13121 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -397,8 +397,6 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
>      struct addrinfo ai, *peer = NULL, *local = NULL;
>      const char *addr;
>      const char *port;
> -    char uaddr[INET6_ADDRSTRLEN+1];
> -    char uport[33];
>      int sock = -1, rc;
>  
>      /* lookup peer addr */
> @@ -413,7 +411,7 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
>          addr = "localhost";
>      }
>      if (port == NULL || strlen(port) == 0) {
> -        fprintf(stderr, "inet_dgram: port not specified\n");
> +        error_setg(errp, "remote port not specified");
>          return -1;
>      }
>  
> @@ -423,8 +421,8 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
>          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_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
> +                   gai_strerror(rc));
>  	return -1;
>      }
>  
> @@ -443,44 +441,28 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
>          port = "0";
>  
>      if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) {
> -        fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> -                gai_strerror(rc));
> +        error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
> +                   gai_strerror(rc));
>          goto err;
>      }
>  
>      /* 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));
> +        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
>          goto err;
>      }
>      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));
> +        error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
>          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_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
>          goto err;
>      }
>  

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

[Luiz Capitulino]

On Wed,  3 Oct 2012 16:37:05 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Before:
> 
>     $ qemu-system-x86_64 -monitor unix:/vvv,server=off
>     connect(unix:/vvv): No such file or directory
>     chardev: opening backend "socket" failed
> 
> After:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -monitor unix:/vvv,server=off
>     qemu-system-x86_64: -monitor unix:/vvv,server=off: Failed to connect to socket: No such file or directory
>     chardev: opening backend "socket" failed
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I'm assuming that clients will print the error to stderr:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  qemu-sockets.c | 12 ++++++------
>  1 file modificato, 6 inserzioni(+), 6 rimozioni(-)
> 
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 55669e9..e28c63c 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -633,7 +633,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> -        perror("socket(unix)");
> +        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>  
> @@ -658,11 +658,11 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>  
>      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_errno(errp, errno, 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_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
>          goto err;
>      }
>  
> @@ -682,7 +682,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
>      int sock, rc;
>  
>      if (NULL == path) {
> -        fprintf(stderr, "unix connect: no path specified\n");
> +        error_setg(errp, "unix connect: no path specified\n");
>          return -1;
>      }
>  
> @@ -694,7 +694,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
>  
>      sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
>      if (sock < 0) {
> -        perror("socket(unix)");
> +        error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
>          return -1;
>      }
>      if (connect_state != NULL) {
> @@ -726,7 +726,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
>      }
>  
>      if (rc < 0) {
> -        fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
> +        error_set_errno(errp, -rc, QERR_SOCKET_CONNECT_FAILED);
>          close(sock);
>  	return -1;
>      }

Re: [Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 04 Oct 2012 18:16:13 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Il 04/10/2012 18:14, Luiz Capitulino ha scritto:
>> >> > +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>> >> > +                     const char *fmt, ...)
>> > 
>> > The function's name makes me expect that something else is done
>> > with os_errno
>>                                                      ^^^^
>> 
>> Why something "else"? :)
>
> What I meant is that, it's not clear from the function's name how os_errno
> is used. Actually, it gives the impression you're storing errno, but I might
> be biased :)
>
> But again, I don't have any better suggestions.

A bit long for my taste, but here goes anyway: error_set_with_errno()

Re: [Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> These functions help maintaining homogeneous formatting of error
> messages that include strerror values.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  error.c | 28 ++++++++++++++++++++++++++++
>  error.h |  9 +++++++++
>  2 file modificati, 37 inserzioni(+)
>
> diff --git a/error.c b/error.c
> index 1f05fc4..128d88c 100644
> --- a/error.c
> +++ b/error.c
> @@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      *errp = err;
>  }
>  
> +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> +                     const char *fmt, ...)
> +{
> +    Error *err;
> +    char *msg1;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +    assert(*errp == NULL);
> +
> +    err = g_malloc0(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    msg1 = g_strdup_vprintf(fmt, ap);
> +    if (os_errno != 0) {
> +        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
> +        g_free(msg1);
> +    } else {
> +        err->msg = msg1;
> +    }
> +    va_end(ap);
> +    err->err_class = err_class;
> +
> +    *errp = err;
> +}
> +

Duplicates error_set() code without need.  How about:

void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                     const char *fmt, ...)
{
    char *msg;

    va_start(ap, fmt);
    msg = g_strdup_printf(fmt, ap);
    va_end(ap);
    if (os_errno) {
        error_set(errp, err_class, "%s: %s", msg, strerror(os_errno));
    } else {
        error_set(errp, err_class, "%s", msg);
    }
    g_free(msg);
}

Sketch, not even compile-tested.

Or the other way round, implement error_set() in terms of the more
general error_set_errno():

void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
{
    error_set(errp, err_class, 0, fmt, ...);
}

Except that's not C; real code needs verror_set_errno().

As usual, every variadic function sooner or later needs a buddy that
takes a va_list instead.

>  Error *error_copy(const Error *err)
>  {
>      Error *err_new;
[...]

Re: [Qemu-devel] [PATCH 01/18] error: add error_set_errno and error_setg_errno

[Paolo Bonzini]

Il 17/10/2012 14:56, Markus Armbruster ha scritto:
> Duplicates error_set() code without need.  How about:
> 
> void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>                      const char *fmt, ...)
> {
>     char *msg;
> 
>     va_start(ap, fmt);
>     msg = g_strdup_printf(fmt, ap);
>     va_end(ap);
>     if (os_errno) {
>         error_set(errp, err_class, "%s: %s", msg, strerror(os_errno));
>     } else {
>         error_set(errp, err_class, "%s", msg);
>     }
>     g_free(msg);
> }
> 
> Sketch, not even compile-tested.
> 
> Or the other way round, implement error_set() in terms of the more
> general error_set_errno():
> 
> void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> {
>     error_set(errp, err_class, 0, fmt, ...);
> }
> 
> Except that's not C; real code needs verror_set_errno().
> 
> As usual, every variadic function sooner or later needs a buddy that
> takes a va_list instead.

Indeed... lazy me.

Added to my todo list.

Paolo

Re: [Qemu-devel] [PATCH 02/18] qemu-sockets: add Error ** to all functions

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH 03/18] qemu-sockets: unix_listen and unix_connect are portable

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> They are just wrappers and do not need a Win32-specific version.

This gave me pause (it's my thick-headed day, apparently), until I
realized that in addition to what the commit message says you also move
related code.  Please try harder to make review as easy as possible.

Re: [Qemu-devel] [PATCH 03/18] qemu-sockets: unix_listen and unix_connect are portable

[Paolo Bonzini]

Il 17/10/2012 15:17, Markus Armbruster ha scritto:
>> > They are just wrappers and do not need a Win32-specific version.
> This gave me pause (it's my thick-headed day, apparently), until I
> realized that in addition to what the commit message says you also move
> related code.  Please try harder to make review as easy as possible.

I don't, it's just git that decided to make the diff this way. :)

Paolo