diff mbox

[v5,07/45] Return path: Open a return path on QEMUFile for sockets

Message ID 1424883128-9841-8-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert Feb. 25, 2015, 4:51 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Postcopy needs a method to send messages from the destination back to
the source, this is the 'return path'.

Wire it up for 'socket' QEMUFile's using a dup'd fd.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/migration/qemu-file.h  |  7 +++++
 migration/qemu-file-internal.h |  2 ++
 migration/qemu-file-unix.c     | 58 +++++++++++++++++++++++++++++++++++-------
 migration/qemu-file.c          | 12 +++++++++
 4 files changed, 70 insertions(+), 9 deletions(-)

Comments

David Gibson March 10, 2015, 2:49 a.m. UTC | #1 
On Wed, Feb 25, 2015 at 04:51:30PM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Postcopy needs a method to send messages from the destination back to
> the source, this is the 'return path'.
> 
> Wire it up for 'socket' QEMUFile's using a dup'd fd.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/migration/qemu-file.h  |  7 +++++
>  migration/qemu-file-internal.h |  2 ++
>  migration/qemu-file-unix.c     | 58 +++++++++++++++++++++++++++++++++++-------
>  migration/qemu-file.c          | 12 +++++++++
>  4 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 6ae0b03..3c38963 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -85,6 +85,11 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
>                                 int *bytes_sent);
>  
>  /*
> + * Return a QEMUFile for comms in the opposite direction
> + */
> +typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> +
> +/*
>   * Stop any read or write (depending on flags) on the underlying
>   * transport on the QEMUFile.
>   * Existing blocking reads/writes must be woken
> @@ -102,6 +107,7 @@ typedef struct QEMUFileOps {
>      QEMURamHookFunc *after_ram_iterate;
>      QEMURamHookFunc *hook_ram_load;
>      QEMURamSaveFunc *save_page;
> +    QEMURetPathFunc *get_return_path;
>      QEMUFileShutdownFunc *shut_down;
>  } QEMUFileOps;
>  
> @@ -188,6 +194,7 @@ int64_t qemu_file_get_rate_limit(QEMUFile *f);
>  int qemu_file_get_error(QEMUFile *f);
>  void qemu_file_set_error(QEMUFile *f, int ret);
>  int qemu_file_shutdown(QEMUFile *f);
> +QEMUFile *qemu_file_get_return_path(QEMUFile *f);
>  void qemu_fflush(QEMUFile *f);
>  
>  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> diff --git a/migration/qemu-file-internal.h b/migration/qemu-file-internal.h
> index d95e853..a39b8e3 100644
> --- a/migration/qemu-file-internal.h
> +++ b/migration/qemu-file-internal.h
> @@ -48,6 +48,8 @@ struct QEMUFile {
>      unsigned int iovcnt;
>  
>      int last_error;
> +
> +    struct QEMUFile *return_path;

AFAICT, the only thing this field is used for is an assert, which
seems a bit pointless.  I'd suggest either getting rid of it, or
make qemu_file_get_return_path() safely idempotent by having it only
call the FileOps pointer if QEMUFile::return_path is non-NULL,
otherwise just return the existing return_path.

Setting the field probably belongs better in the wrapper than in the
socket specific callback, too, since there's nothing inherently
related to the socket implementation about it.
Dr. David Alan Gilbert March 13, 2015, 1:14 p.m. UTC | #2 
* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Wed, Feb 25, 2015 at 04:51:30PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Postcopy needs a method to send messages from the destination back to
> > the source, this is the 'return path'.
> > 
> > Wire it up for 'socket' QEMUFile's using a dup'd fd.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  include/migration/qemu-file.h  |  7 +++++
> >  migration/qemu-file-internal.h |  2 ++
> >  migration/qemu-file-unix.c     | 58 +++++++++++++++++++++++++++++++++++-------
> >  migration/qemu-file.c          | 12 +++++++++
> >  4 files changed, 70 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> > index 6ae0b03..3c38963 100644
> > --- a/include/migration/qemu-file.h
> > +++ b/include/migration/qemu-file.h
> > @@ -85,6 +85,11 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
> >                                 int *bytes_sent);
> >  
> >  /*
> > + * Return a QEMUFile for comms in the opposite direction
> > + */
> > +typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
> > +
> > +/*
> >   * Stop any read or write (depending on flags) on the underlying
> >   * transport on the QEMUFile.
> >   * Existing blocking reads/writes must be woken
> > @@ -102,6 +107,7 @@ typedef struct QEMUFileOps {
> >      QEMURamHookFunc *after_ram_iterate;
> >      QEMURamHookFunc *hook_ram_load;
> >      QEMURamSaveFunc *save_page;
> > +    QEMURetPathFunc *get_return_path;
> >      QEMUFileShutdownFunc *shut_down;
> >  } QEMUFileOps;
> >  
> > @@ -188,6 +194,7 @@ int64_t qemu_file_get_rate_limit(QEMUFile *f);
> >  int qemu_file_get_error(QEMUFile *f);
> >  void qemu_file_set_error(QEMUFile *f, int ret);
> >  int qemu_file_shutdown(QEMUFile *f);
> > +QEMUFile *qemu_file_get_return_path(QEMUFile *f);
> >  void qemu_fflush(QEMUFile *f);
> >  
> >  static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
> > diff --git a/migration/qemu-file-internal.h b/migration/qemu-file-internal.h
> > index d95e853..a39b8e3 100644
> > --- a/migration/qemu-file-internal.h
> > +++ b/migration/qemu-file-internal.h
> > @@ -48,6 +48,8 @@ struct QEMUFile {
> >      unsigned int iovcnt;
> >  
> >      int last_error;
> > +
> > +    struct QEMUFile *return_path;
> 
> AFAICT, the only thing this field is used for is an assert, which
> seems a bit pointless.  I'd suggest either getting rid of it, or

Done; it's gone.

Dave

> make qemu_file_get_return_path() safely idempotent by having it only
> call the FileOps pointer if QEMUFile::return_path is non-NULL,
> otherwise just return the existing return_path.
> 
> Setting the field probably belongs better in the wrapper than in the
> socket specific callback, too, since there's nothing inherently
> related to the socket implementation about it.
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 6ae0b03..3c38963 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -85,6 +85,11 @@  typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
                                int *bytes_sent);
 
 /*
+ * Return a QEMUFile for comms in the opposite direction
+ */
+typedef QEMUFile *(QEMURetPathFunc)(void *opaque);
+
+/*
  * Stop any read or write (depending on flags) on the underlying
  * transport on the QEMUFile.
  * Existing blocking reads/writes must be woken
@@ -102,6 +107,7 @@  typedef struct QEMUFileOps {
     QEMURamHookFunc *after_ram_iterate;
     QEMURamHookFunc *hook_ram_load;
     QEMURamSaveFunc *save_page;
+    QEMURetPathFunc *get_return_path;
     QEMUFileShutdownFunc *shut_down;
 } QEMUFileOps;
 
@@ -188,6 +194,7 @@  int64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_get_error(QEMUFile *f);
 void qemu_file_set_error(QEMUFile *f, int ret);
 int qemu_file_shutdown(QEMUFile *f);
+QEMUFile *qemu_file_get_return_path(QEMUFile *f);
 void qemu_fflush(QEMUFile *f);
 
 static inline void qemu_put_be64s(QEMUFile *f, const uint64_t *pv)
diff --git a/migration/qemu-file-internal.h b/migration/qemu-file-internal.h
index d95e853..a39b8e3 100644
--- a/migration/qemu-file-internal.h
+++ b/migration/qemu-file-internal.h
@@ -48,6 +48,8 @@  struct QEMUFile {
     unsigned int iovcnt;
 
     int last_error;
+
+    struct QEMUFile *return_path;
 };
 
 #endif
diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c
index bfbc086..50291cf 100644
--- a/migration/qemu-file-unix.c
+++ b/migration/qemu-file-unix.c
@@ -96,6 +96,45 @@  static int socket_shutdown(void *opaque, bool rd, bool wr)
     }
 }
 
+/*
+ * Give a QEMUFile* off the same socket but data in the opposite
+ * direction.
+ */
+static QEMUFile *socket_dup_return_path(void *opaque)
+{
+    QEMUFileSocket *qfs = opaque;
+    int revfd;
+    bool this_is_read;
+    QEMUFile *result;
+
+    /* We should only be called once to get a RP on a file */
+    assert(!qfs->file->return_path);
+
+    if (qemu_file_get_error(qfs->file)) {
+        /* If the forward file is in error, don't try and open a return */
+        return NULL;
+    }
+
+    /* I don't think there's a better way to tell which direction 'this' is */
+    this_is_read = qfs->file->ops->get_buffer != NULL;
+
+    revfd = dup(qfs->fd);
+    if (revfd == -1) {
+        error_report("Error duplicating fd for return path: %s",
+                      strerror(errno));
+        return NULL;
+    }
+
+    result = qemu_fopen_socket(revfd, this_is_read ? "wb" : "rb");
+    qfs->file->return_path = result;
+
+    if (!result) {
+        close(revfd);
+    }
+
+    return result;
+}
+
 static ssize_t unix_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
                                   int64_t pos)
 {
@@ -204,18 +243,19 @@  QEMUFile *qemu_fdopen(int fd, const char *mode)
 }
 
 static const QEMUFileOps socket_read_ops = {
-    .get_fd     = socket_get_fd,
-    .get_buffer = socket_get_buffer,
-    .close      = socket_close,
-    .shut_down  = socket_shutdown
-
+    .get_fd          = socket_get_fd,
+    .get_buffer      = socket_get_buffer,
+    .close           = socket_close,
+    .shut_down       = socket_shutdown,
+    .get_return_path = socket_dup_return_path
 };
 
 static const QEMUFileOps socket_write_ops = {
-    .get_fd        = socket_get_fd,
-    .writev_buffer = socket_writev_buffer,
-    .close         = socket_close,
-    .shut_down     = socket_shutdown
+    .get_fd          = socket_get_fd,
+    .writev_buffer   = socket_writev_buffer,
+    .close           = socket_close,
+    .shut_down       = socket_shutdown,
+    .get_return_path = socket_dup_return_path
 };
 
 QEMUFile *qemu_fopen_socket(int fd, const char *mode)
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 57eb868..02122a5 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -42,6 +42,18 @@  int qemu_file_shutdown(QEMUFile *f)
     return f->ops->shut_down(f->opaque, true, true);
 }
 
+/*
+ * Result: QEMUFile* for a 'return path' for comms in the opposite direction
+ *         NULL if not available
+ */
+QEMUFile *qemu_file_get_return_path(QEMUFile *f)
+{
+    if (!f->ops->get_return_path) {
+        return NULL;
+    }
+    return f->ops->get_return_path(f->opaque);
+}
+
 bool qemu_file_mode_is_not_valid(const char *mode)
 {
     if (mode == NULL ||