Patchwork [08/25] migration (outgoing): add error propagation for all protocols

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 10, 2012, 2:02 p.m.
Message ID <1349877786-23514-9-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/190691/
State New
Headers show

Comments

Paolo Bonzini - Oct. 10, 2012, 2:02 p.m.
Error propagation is already there for socket backends, even though
it is (and remains) incomplete because no Error is passed to the
NonBlockingConnectHandler.  Add it to other protocols, simplifying
code that tests for errors that will never happen.

With all protocols understanding Error, the code can be simplified
further 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>
---
        v1->v2: turn bizarre DPRINTF into an assertion failure or just
        drop it for the failure test of O_NONBLOCK. Clean up after this
        change.

 migration-fd.c   | 19 ++++---------------
 migration-tcp.c  | 13 ++-----------
 migration-unix.c | 11 ++---------
 migration.c      | 17 ++++++-----------
 migration.h      |  9 ++++-----
 6 file modificati, 22 inserzioni(+), 65 rimozioni(-)
Markus Armbruster - Oct. 17, 2012, 2:43 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

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

Why is that a problem?

>                             Add it to other protocols, simplifying
> code that tests for errors that will never happen.
>
> With all protocols understanding Error, the code can be simplified
> further 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>
> ---
>         v1->v2: turn bizarre DPRINTF into an assertion failure or just
>         drop it for the failure test of O_NONBLOCK. Clean up after this
>         change.
>
>  migration-fd.c   | 19 ++++---------------
>  migration-tcp.c  | 13 ++-----------
>  migration-unix.c | 11 ++---------
>  migration.c      | 17 ++++++-----------
>  migration.h      |  9 ++++-----
>  6 file modificati, 22 inserzioni(+), 65 rimozioni(-)
>
> diff --git a/migration-exec.c b/migration-exec.c
> index 6c97db9..5f3f4b2 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -60,22 +60,18 @@ 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;
> +        error_setg_errno(errp, errno, "failed to popen the migration target");
> +        return;
>      }
>  
>      s->fd = fileno(f);
> -    if (s->fd == -1) {
> -        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
> -        goto err_after_open;
> -    }
> -
> +    assert(s->fd != -1);

Okay, because fileno() can fail only if its argument is not a valid
stream, which really shouldn't be possible here.

>      socket_set_nonblock(s->fd);
>  
>      s->opaque = qemu_popen(f, "w");
> @@ -85,12 +81,6 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
>      s->write = file_write;
>  
>      migrate_fd_connect(s);
> -    return 0;
> -
> -err_after_open:
> -    pclose(f);
> -err_after_popen:
> -    return -1;
>  }
>  
>  static void exec_accept_incoming_migration(void *opaque)
> diff --git a/migration-fd.c b/migration-fd.c
> index 7335167..a7c800a 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -73,30 +73,19 @@ 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;
> -    }
> -
> -    if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
> -        DPRINTF("Unable to set nonblocking mode on file descriptor\n");
> -        goto err_after_open;
> +        return;
>      }
>  
> +    fcntl(s->fd, F_SETFL, O_NONBLOCK);

Sure this can't fail?

>      s->get_error = fd_errno;
>      s->write = fd_write;
>      s->close = fd_close;
>  
>      migrate_fd_connect(s);
> -    return 0;
> -
> -err_after_open:
> -    close(s->fd);
> -err_after_get_fd:
> -    return -1;
>  }
>  
>  static void fd_accept_incoming_migration(void *opaque)
[...]
Paolo Bonzini - Oct. 17, 2012, 2:50 p.m.
Il 17/10/2012 16:43, Markus Armbruster ha scritto:
>> > Error propagation is already there for socket backends, even though
>> > it is (and remains) incomplete because no Error is passed to the
>> > NonBlockingConnectHandler.
> Why is that a problem?

It means that the exact error message still cannot be sent to the user
if the OS reports it asynchronously via SO_ERROR.  If
NonBlockingConnectHandler received an Error**, we could for example
report the error class and/or message via a new field of the
query-migration command even if it is reported asynchronously.

Paolo
Markus Armbruster - Oct. 17, 2012, 3:43 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 17/10/2012 16:43, Markus Armbruster ha scritto:
>>> > Error propagation is already there for socket backends, even though
>>> > it is (and remains) incomplete because no Error is passed to the
>>> > NonBlockingConnectHandler.
>> Why is that a problem?
>
> It means that the exact error message still cannot be sent to the user
> if the OS reports it asynchronously via SO_ERROR.  If
> NonBlockingConnectHandler received an Error**, we could for example
> report the error class and/or message via a new field of the
> query-migration command even if it is reported asynchronously.

First time you mention the problem in a commit message could use a
variant of this explanation.

Patch

diff --git a/migration-exec.c b/migration-exec.c
index 6c97db9..5f3f4b2 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -60,22 +60,18 @@  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;
+        error_setg_errno(errp, errno, "failed to popen the migration target");
+        return;
     }
 
     s->fd = fileno(f);
-    if (s->fd == -1) {
-        DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
-        goto err_after_open;
-    }
-
+    assert(s->fd != -1);
     socket_set_nonblock(s->fd);
 
     s->opaque = qemu_popen(f, "w");
@@ -85,12 +81,6 @@  int exec_start_outgoing_migration(MigrationState *s, const char *command)
     s->write = file_write;
 
     migrate_fd_connect(s);
-    return 0;
-
-err_after_open:
-    pclose(f);
-err_after_popen:
-    return -1;
 }
 
 static void exec_accept_incoming_migration(void *opaque)
diff --git a/migration-fd.c b/migration-fd.c
index 7335167..a7c800a 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -73,30 +73,19 @@  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;
-    }
-
-    if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
-        DPRINTF("Unable to set nonblocking mode on file descriptor\n");
-        goto err_after_open;
+        return;
     }
 
+    fcntl(s->fd, F_SETFL, O_NONBLOCK);
     s->get_error = fd_errno;
     s->write = fd_write;
     s->close = fd_close;
 
     migrate_fd_connect(s);
-    return 0;
-
-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);