Patchwork [35/41] migration: use QEMUFile for writing outgoing migration data

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 15, 2013, 5:47 p.m.
Message ID <1360950433-17106-36-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/220819/
State New
Headers show

Comments

Paolo Bonzini - Feb. 15, 2013, 5:47 p.m.
Second, drop the file descriptor indirection, and write directly to the
QEMUFile.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/migration/migration.h |    4 ---
 migration-exec.c              |   12 -----------
 migration-fd.c                |   12 -----------
 migration-tcp.c               |   12 -----------
 migration-unix.c              |   12 -----------
 migration.c                   |   44 +++++-----------------------------------
 6 files changed, 6 insertions(+), 90 deletions(-)
Orit Wasserman - Feb. 21, 2013, 8:58 a.m.
On 02/15/2013 07:47 PM, Paolo Bonzini wrote:
> Second, drop the file descriptor indirection, and write directly to the
> QEMUFile.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/migration/migration.h |    4 ---
>  migration-exec.c              |   12 -----------
>  migration-fd.c                |   12 -----------
>  migration-tcp.c               |   12 -----------
>  migration-unix.c              |   12 -----------
>  migration.c                   |   44 +++++-----------------------------------
>  6 files changed, 6 insertions(+), 90 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index cf3e81c..bde13c2 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -40,10 +40,6 @@ struct MigrationState
>      QEMUFile *file;
>      QEMUFile *migration_file;
>  
> -    int fd;
> -    int (*get_error)(MigrationState *s);
> -    int (*write)(MigrationState *s, const void *buff, size_t size);
> -
>      int state;
>      MigrationParams params;
>      int64_t total_time;
> diff --git a/migration-exec.c b/migration-exec.c
> index 8c3f720..1c539de 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -33,16 +33,6 @@
>      do { } while (0)
>  #endif
>  
> -static int file_errno(MigrationState *s)
> -{
> -    return errno;
> -}
> -
> -static int file_write(MigrationState *s, const void * buf, size_t size)
> -{
> -    return write(s->fd, buf, size);
> -}
> -
>  void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>  {
>      s->migration_file = qemu_popen_cmd(command, "w");
> @@ -51,8 +41,6 @@ void exec_start_outgoing_migration(MigrationState *s, const char *command, Error
>          return;
>      }
>  
> -    s->get_error = file_errno;
> -    s->write = file_write;
>      migrate_fd_connect(s);
>  }
>  
> diff --git a/migration-fd.c b/migration-fd.c
> index 4636457..07c758a 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -30,16 +30,6 @@
>      do { } while (0)
>  #endif
>  
> -static int fd_errno(MigrationState *s)
> -{
> -    return errno;
> -}
> -
> -static int fd_write(MigrationState *s, const void * buf, size_t size)
> -{
> -    return write(s->fd, buf, size);
> -}
> -
>  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
>  {
>      int fd = monitor_get_fd(cur_mon, fdname, errp);
> @@ -48,8 +38,6 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
>      }
>      s->migration_file = qemu_fdopen(fd, "wb");
>  
> -    s->get_error = fd_errno;
> -    s->write = fd_write;
>      migrate_fd_connect(s);
>  }
>  
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 1e8e004..5ea4f3d 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -29,16 +29,6 @@
>      do { } while (0)
>  #endif
>  
> -static int socket_errno(MigrationState *s)
> -{
> -    return socket_error();
> -}
> -
> -static int socket_write(MigrationState *s, const void * buf, size_t size)
> -{
> -    return send(s->fd, buf, size, 0);
> -}
> -
>  static void tcp_wait_for_connect(int fd, void *opaque)
>  {
>      MigrationState *s = opaque;
> @@ -56,8 +46,6 @@ static void tcp_wait_for_connect(int fd, void *opaque)
>  
>  void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp)
>  {
> -    s->get_error = socket_errno;
> -    s->write = socket_write;
>      inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp);
>  }
>  
> diff --git a/migration-unix.c b/migration-unix.c
> index 11917f4..64bfa31 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -29,16 +29,6 @@
>      do { } while (0)
>  #endif
>  
> -static int unix_errno(MigrationState *s)
> -{
> -    return errno;
> -}
> -
> -static int unix_write(MigrationState *s, const void * buf, size_t size)
> -{
> -    return write(s->fd, buf, size);
> -}
> -
>  static void unix_wait_for_connect(int fd, void *opaque)
>  {
>      MigrationState *s = opaque;
> @@ -56,8 +46,6 @@ static void unix_wait_for_connect(int fd, void *opaque)
>  
>  void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
>  {
> -    s->get_error = unix_errno;
> -    s->write = unix_write;
>      unix_nonblocking_connect(path, unix_wait_for_connect, s, errp);
>  }
>  
> diff --git a/migration.c b/migration.c
> index 9cffdd4..68d47cd 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -291,25 +291,6 @@ void migrate_fd_error(MigrationState *s)
>      notifier_list_notify(&migration_state_notifiers, s);
>  }
>  
> -static ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
> -                                     size_t size)
> -{
> -    ssize_t ret;
> -
> -    if (s->state != MIG_STATE_ACTIVE) {
> -        return -EIO;
> -    }
> -
> -    do {
> -        ret = s->write(s, data, size);
> -    } while (ret == -1 && ((s->get_error(s)) == EINTR));
> -
> -    if (ret == -1)
> -        ret = -(s->get_error(s));
> -
> -    return ret;
> -}
> -
>  static void migrate_fd_cancel(MigrationState *s)
>  {
>      DPRINTF("cancelling migration\n");
> @@ -323,7 +304,6 @@ int migrate_fd_close(MigrationState *s)
>      if (s->migration_file != NULL) {
>          rc = qemu_fclose(s->migration_file);
>          s->migration_file = NULL;
> -        s->fd = -1;
>      }
>      return rc;
>  }
> @@ -508,8 +488,6 @@ static int migration_put_buffer(void *opaque, const uint8_t *buf,
>                                 int64_t pos, int size)
>  {
>      MigrationState *s = opaque;
> -    ssize_t ret;
> -    size_t sent;
>  
>      DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
>  
> @@ -517,22 +495,13 @@ static int migration_put_buffer(void *opaque, const uint8_t *buf,
>          return size;
>      }
>  
> -    sent = 0;
> -    while (size) {
> -        ret = migrate_fd_put_buffer(s, buf, size);
> -        if (ret <= 0) {
> -            DPRINTF("error flushing data, %zd\n", ret);
> -            return ret;
> -        } else {
> -            DPRINTF("flushed %zd byte(s)\n", ret);
> -            sent += ret;
> -            buf += ret;
> -            size -= ret;
> -            s->bytes_xfer += ret;
> -        }
> +    qemu_put_buffer(s->migration_file, buf, size);
> +    if (qemu_file_get_error(s->migration_file)) {
> +        return qemu_file_get_error(s->migration_file);
>      }
>  
> -    return sent;
> +    s->bytes_xfer += size;
> +    return size;
>  }
>  
>  static int migration_close(void *opaque)
> @@ -553,7 +522,7 @@ static int migration_get_fd(void *opaque)
>  {
>      MigrationState *s = opaque;
>  
> -    return s->fd;
> +    return qemu_get_fd(s->migration_file);
>  }
>  
>  /*
> @@ -695,7 +664,6 @@ void migrate_fd_connect(MigrationState *s)
>      s->xfer_limit = s->bandwidth_limit / XFER_LIMIT_RATIO;
>  
>      s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
> -    s->fd = qemu_get_fd(s->migration_file);
>      s->file = qemu_fopen_ops(s, &migration_file_ops);
>  
>      qemu_thread_create(&s->thread, migration_thread, s,
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>
Juan Quintela - Feb. 22, 2013, 11:30 a.m.
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Second, drop the file descriptor indirection, and write directly to the
> QEMUFile.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> +    qemu_put_buffer(s->migration_file, buf, size);
> +    if (qemu_file_get_error(s->migration_file)) {
> +        return qemu_file_get_error(s->migration_file);

Rest of patch is really, really nice.

But here, please, use a local variable.

    qemu_put_buffer(s->migration_file, buf, size);
    ret = qemu_file_get_error(s->migration_file);
    if (ret) {
       return ret;
    }
Juan Quintela - Feb. 22, 2013, 11:55 a.m.
Juan Quintela <quintela@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Second, drop the file descriptor indirection, and write directly to the
>> QEMUFile.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
>> +    qemu_put_buffer(s->migration_file, buf, size);
>> +    if (qemu_file_get_error(s->migration_file)) {
>> +        return qemu_file_get_error(s->migration_file);
>
> Rest of patch is really, really nice.
>
> But here, please, use a local variable.
>
>     qemu_put_buffer(s->migration_file, buf, size);
>     ret = qemu_file_get_error(s->migration_file);
>     if (ret) {
>        return ret;
>     }

This chunk is removed in a later patch, so don't care about changing it.

Reviewed-by: Juan Quintela <quintela@redhat.com>

Patch

diff --git a/include/migration/migration.h b/include/migration/migration.h
index cf3e81c..bde13c2 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -40,10 +40,6 @@  struct MigrationState
     QEMUFile *file;
     QEMUFile *migration_file;
 
-    int fd;
-    int (*get_error)(MigrationState *s);
-    int (*write)(MigrationState *s, const void *buff, size_t size);
-
     int state;
     MigrationParams params;
     int64_t total_time;
diff --git a/migration-exec.c b/migration-exec.c
index 8c3f720..1c539de 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -33,16 +33,6 @@ 
     do { } while (0)
 #endif
 
-static int file_errno(MigrationState *s)
-{
-    return errno;
-}
-
-static int file_write(MigrationState *s, const void * buf, size_t size)
-{
-    return write(s->fd, buf, size);
-}
-
 void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
 {
     s->migration_file = qemu_popen_cmd(command, "w");
@@ -51,8 +41,6 @@  void exec_start_outgoing_migration(MigrationState *s, const char *command, Error
         return;
     }
 
-    s->get_error = file_errno;
-    s->write = file_write;
     migrate_fd_connect(s);
 }
 
diff --git a/migration-fd.c b/migration-fd.c
index 4636457..07c758a 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -30,16 +30,6 @@ 
     do { } while (0)
 #endif
 
-static int fd_errno(MigrationState *s)
-{
-    return errno;
-}
-
-static int fd_write(MigrationState *s, const void * buf, size_t size)
-{
-    return write(s->fd, buf, size);
-}
-
 void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
 {
     int fd = monitor_get_fd(cur_mon, fdname, errp);
@@ -48,8 +38,6 @@  void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
     }
     s->migration_file = qemu_fdopen(fd, "wb");
 
-    s->get_error = fd_errno;
-    s->write = fd_write;
     migrate_fd_connect(s);
 }
 
diff --git a/migration-tcp.c b/migration-tcp.c
index 1e8e004..5ea4f3d 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -29,16 +29,6 @@ 
     do { } while (0)
 #endif
 
-static int socket_errno(MigrationState *s)
-{
-    return socket_error();
-}
-
-static int socket_write(MigrationState *s, const void * buf, size_t size)
-{
-    return send(s->fd, buf, size, 0);
-}
-
 static void tcp_wait_for_connect(int fd, void *opaque)
 {
     MigrationState *s = opaque;
@@ -56,8 +46,6 @@  static void tcp_wait_for_connect(int fd, void *opaque)
 
 void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp)
 {
-    s->get_error = socket_errno;
-    s->write = socket_write;
     inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp);
 }
 
diff --git a/migration-unix.c b/migration-unix.c
index 11917f4..64bfa31 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -29,16 +29,6 @@ 
     do { } while (0)
 #endif
 
-static int unix_errno(MigrationState *s)
-{
-    return errno;
-}
-
-static int unix_write(MigrationState *s, const void * buf, size_t size)
-{
-    return write(s->fd, buf, size);
-}
-
 static void unix_wait_for_connect(int fd, void *opaque)
 {
     MigrationState *s = opaque;
@@ -56,8 +46,6 @@  static void unix_wait_for_connect(int fd, void *opaque)
 
 void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
 {
-    s->get_error = unix_errno;
-    s->write = unix_write;
     unix_nonblocking_connect(path, unix_wait_for_connect, s, errp);
 }
 
diff --git a/migration.c b/migration.c
index 9cffdd4..68d47cd 100644
--- a/migration.c
+++ b/migration.c
@@ -291,25 +291,6 @@  void migrate_fd_error(MigrationState *s)
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
-static ssize_t migrate_fd_put_buffer(MigrationState *s, const void *data,
-                                     size_t size)
-{
-    ssize_t ret;
-
-    if (s->state != MIG_STATE_ACTIVE) {
-        return -EIO;
-    }
-
-    do {
-        ret = s->write(s, data, size);
-    } while (ret == -1 && ((s->get_error(s)) == EINTR));
-
-    if (ret == -1)
-        ret = -(s->get_error(s));
-
-    return ret;
-}
-
 static void migrate_fd_cancel(MigrationState *s)
 {
     DPRINTF("cancelling migration\n");
@@ -323,7 +304,6 @@  int migrate_fd_close(MigrationState *s)
     if (s->migration_file != NULL) {
         rc = qemu_fclose(s->migration_file);
         s->migration_file = NULL;
-        s->fd = -1;
     }
     return rc;
 }
@@ -508,8 +488,6 @@  static int migration_put_buffer(void *opaque, const uint8_t *buf,
                                int64_t pos, int size)
 {
     MigrationState *s = opaque;
-    ssize_t ret;
-    size_t sent;
 
     DPRINTF("putting %d bytes at %" PRId64 "\n", size, pos);
 
@@ -517,22 +495,13 @@  static int migration_put_buffer(void *opaque, const uint8_t *buf,
         return size;
     }
 
-    sent = 0;
-    while (size) {
-        ret = migrate_fd_put_buffer(s, buf, size);
-        if (ret <= 0) {
-            DPRINTF("error flushing data, %zd\n", ret);
-            return ret;
-        } else {
-            DPRINTF("flushed %zd byte(s)\n", ret);
-            sent += ret;
-            buf += ret;
-            size -= ret;
-            s->bytes_xfer += ret;
-        }
+    qemu_put_buffer(s->migration_file, buf, size);
+    if (qemu_file_get_error(s->migration_file)) {
+        return qemu_file_get_error(s->migration_file);
     }
 
-    return sent;
+    s->bytes_xfer += size;
+    return size;
 }
 
 static int migration_close(void *opaque)
@@ -553,7 +522,7 @@  static int migration_get_fd(void *opaque)
 {
     MigrationState *s = opaque;
 
-    return s->fd;
+    return qemu_get_fd(s->migration_file);
 }
 
 /*
@@ -695,7 +664,6 @@  void migrate_fd_connect(MigrationState *s)
     s->xfer_limit = s->bandwidth_limit / XFER_LIMIT_RATIO;
 
     s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
-    s->fd = qemu_get_fd(s->migration_file);
     s->file = qemu_fopen_ops(s, &migration_file_ops);
 
     qemu_thread_create(&s->thread, migration_thread, s,