From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40138) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X3Mra-0002EH-Bv for qemu-devel@nongnu.org; Sat, 05 Jul 2014 06:06:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X3MrN-0007PA-88 for qemu-devel@nongnu.org; Sat, 05 Jul 2014 06:06:34 -0400 Received: from mail-qg0-x234.google.com ([2607:f8b0:400d:c04::234]:63499) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X3MrN-0007P3-2a for qemu-devel@nongnu.org; Sat, 05 Jul 2014 06:06:21 -0400 Received: by mail-qg0-f52.google.com with SMTP id f51so2115770qge.39 for ; Sat, 05 Jul 2014 03:06:20 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <53B7CE18.9020907@redhat.com> Date: Sat, 05 Jul 2014 12:06:16 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1404495717-4239-1-git-send-email-dgilbert@redhat.com> <1404495717-4239-8-git-send-email-dgilbert@redhat.com> In-Reply-To: <1404495717-4239-8-git-send-email-dgilbert@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/46] Return path: Open a return path on QEMUFile for sockets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert (git)" , qemu-devel@nongnu.org Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, lilei@linux.vnet.ibm.com, quintela@redhat.com Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto: > From: "Dr. David Alan Gilbert" > > 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 > --- > include/migration/qemu-file.h | 8 +++++ > qemu-file.c | 74 +++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 76 insertions(+), 6 deletions(-) > > diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h > index df38646..ec1a342 100644 > --- a/include/migration/qemu-file.h > +++ b/include/migration/qemu-file.h > @@ -87,6 +87,11 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque, > #define QEMUFILE_IO_BUF_SIZE 32768 > #define QEMUFILE_MAX_IOV_SIZE MIN(IOV_MAX, 64) > > +/* > + * Return a QEMUFile for comms in the opposite direction > + */ > +typedef QEMUFile *(QEMURetPathFunc)(void *opaque); > + > typedef struct QEMUFileOps { > QEMUFilePutBufferFunc *put_buffer; > QEMUFileGetBufferFunc *get_buffer; > @@ -97,6 +102,7 @@ typedef struct QEMUFileOps { > QEMURamHookFunc *after_ram_iterate; > QEMURamHookFunc *hook_ram_load; > QEMURamSaveFunc *save_page; > + QEMURetPathFunc *get_return_path; > } QEMUFileOps; > > struct QEMUFile { > @@ -117,6 +123,7 @@ struct QEMUFile { > > int last_error; > > + struct QEMUFile *return_path; > MigrationIncomingState *mis; > }; > > @@ -202,6 +209,7 @@ void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate); > 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); > +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 88cacc7..98a6d2a 100644 > --- a/qemu-file.c > +++ b/qemu-file.c > @@ -16,6 +16,54 @@ typedef struct QEMUFileSocket { > QEMUFile *file; > } QEMUFileSocket; > > +/* 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? > + } > + > + 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? In other words, can you draw a table with source/dest and read/write, and whether it should be blocking or non-blocking? 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 || >