Re: [Qemu-devel] [PATCH 07/46] Return path: Open a return path on QEMUFile for sockets

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto:
> >From: "Dr. David Alan Gilbert" <address@hidden>
> >
> >Postcopy needs a method to send messages from the destination back to
> >the source, this is the 'return path'.
> >

<snip>

> >+/* Give a QEMUFile* off the same socket but data in the opposite
> >+ * direction.
> >+ * qemu_fopen_socket marks write fd's as blocking, but doesn't
> >+ * touch read fd's status, so we dup the fd just to keep settings
> >+ * separate. [TBD: Do I need to explicitly mark as non-block on read?]
> >+ */
> >+static QEMUFile *socket_dup_return_path(void *opaque)
> >+{
> >+    QEMUFileSocket *qfs = opaque;
> >+    int revfd;
> >+    bool this_is_read;
> >+    QEMUFile *result;
> >+
> >+    /* If it's already open, return it */
> >+    if (qfs->file->return_path) {
> >+        return qfs->file->return_path;
> 
> Wouldn't this leave a dangling file descriptor if you call
> socket_dup_return_path twice, and then close the original QEMUFile?

Hmm - how?

> >+    }
> >+
> >+    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;
> >+    }
> >+
> >+    qemu_set_nonblock(revfd);
> 
> Blocking/nonblocking is per-file *description*, not descriptor.  So you're
> making the original QEMUFile nonblocking as well.  Can you explain why this
> is needed before I reach the meat of the patch series?

Yes, I went through that a few times until I got that it was per-entity not
the fd itself, it still makes life easier for the rest of the QEMUFile
code to have a separate fd for it (hence still worth the dup).

> In other words, can you draw a table with source/dest and read/write, and
> whether it should be blocking or non-blocking?

Sure; the non-blocking ness is mostly on the source side;
modifying the table in the docs patch a little:

  Source side
     Forward path - written by migration thread
            : It's OK for this to be blocking, but we end up with it being
              non-blocking, and modify the socket code to emulate blocking.

     Return path  - opened by main thread, read by fd_handler on main thread
            : Must be non-blocking so as not to block the main thread while
              waiting for a partially sent command.

  Destination side
     Forward path - read by main thread
     Return path  - opened by main thread, written by main thread AND postcopy
                    thread (protected by rp_mutex)

     I think I'm OK with both these being blocking.

Dave

> 
> Paolo
> 
> >+    result = qemu_fopen_socket(revfd, this_is_read ? "wb" : "rb");
> >+    qfs->file->return_path = result;
> >+
> >+    if (result) {
> >+        /* We are the reverse path of our reverse path (although I don't
> >+           expect this to be used, it would stop another dup if it was */
> >+        result->return_path = qfs->file;
> >+    } else {
> >+        close(revfd);
> >+    }
> >+
> >+    return result;
> >+}
> >+
> > static ssize_t socket_writev_buffer(void *opaque, struct iovec *iov, int 
> > iovcnt,
> >                                     int64_t pos)
> > {
> >@@ -313,17 +361,31 @@ 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
> >+    .get_fd          = socket_get_fd,
> >+    .get_buffer      = socket_get_buffer,
> >+    .close           = socket_close,
> >+    .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
> >+    .get_fd          = socket_get_fd,
> >+    .writev_buffer   = socket_writev_buffer,
> >+    .close           = socket_close,
> >+    .get_return_path = socket_dup_return_path
> > };
> >
> >+/*
> >+ * 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 ||
> >
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK