diff mbox

[v4,10/47] Return path: Open a return path on QEMUFile for sockets

Message ID 1412358473-31398-11-git-send-email-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert Oct. 3, 2014, 5:47 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 +++++
 qemu-file.c                   | 73 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 70 insertions(+), 10 deletions(-)

Comments

David Gibson Nov. 3, 2014, 3:05 a.m. UTC | #1
On Fri, Oct 03, 2014 at 06:47:16PM +0100, 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.

This doesn't seem like the right abstraction to me.  In particular I
can't really see how you'd implement this for anything other than
socket.

I'd suggest instead creating new "open" helper functions (within the
QEMUFile code) that open both a forward and return path
simultaneously.
Dr. David Alan Gilbert Nov. 3, 2014, 7:04 p.m. UTC | #2
* David Gibson (david@gibson.dropbear.id.au) wrote:
> On Fri, Oct 03, 2014 at 06:47:16PM +0100, 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.
> 
> This doesn't seem like the right abstraction to me.  In particular I
> can't really see how you'd implement this for anything other than
> socket.
> 
> I'd suggest instead creating new "open" helper functions (within the
> QEMUFile code) that open both a forward and return path
> simultaneously.

Can you give an example of a transport where it would be a problem,
so I can look at how that works?

It's a little tricky since, on the destination, at the time we create
the connection we don't know that we're going to need the return path.

Dave

> -- 
> 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
David Gibson Nov. 18, 2014, 4:34 a.m. UTC | #3
On Mon, Nov 03, 2014 at 07:04:48PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (david@gibson.dropbear.id.au) wrote:
> > On Fri, Oct 03, 2014 at 06:47:16PM +0100, 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.
> > 
> > This doesn't seem like the right abstraction to me.  In particular I
> > can't really see how you'd implement this for anything other than
> > socket.
> > 
> > I'd suggest instead creating new "open" helper functions (within the
> > QEMUFile code) that open both a forward and return path
> > simultaneously.
> 
> Can you give an example of a transport where it would be a problem,
> so I can look at how that works?

pipe

socket routed through some external transport / encoding layer that's
one way only.

> It's a little tricky since, on the destination, at the time we create
> the connection we don't know that we're going to need the return path.

Creating it and not using it shouldn't be a problem though, should it?
As long as you just fall back to precopy rather than failing the
migration if you're unable to open it.
diff mbox

Patch

diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 935cf42..210e9c3 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);
 void 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/qemu-file.c b/qemu-file.c
index 14dcf34..7393415 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -26,6 +26,8 @@  struct QEMUFile {
     unsigned int iovcnt;
 
     int last_error;
+
+    struct QEMUFile *return_path;
 };
 
 typedef struct QEMUFileStdio {
@@ -38,6 +40,45 @@  typedef struct QEMUFileSocket {
     QEMUFile *file;
 } QEMUFileSocket;
 
+/*
+ * 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 socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
                                     int64_t pos)
 {
@@ -343,19 +384,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       = qemufile_socket_shutdown
-
+    .get_fd          = socket_get_fd,
+    .get_buffer      = socket_get_buffer,
+    .close           = socket_close,
+    .shut_down       = qemufile_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       = qemufile_socket_shutdown
-
+    .get_fd          = socket_get_fd,
+    .writev_buffer   = socket_writev_buffer,
+    .close           = socket_close,
+    .shut_down       = qemufile_socket_shutdown,
+    .get_return_path = socket_dup_return_path
 };
 
 /*
@@ -369,6 +410,18 @@  void qemu_file_shutdown(QEMUFile *f)
     f->ops->shut_down(f, 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 ||