* Paolo Bonzini (pbonz...@redhat.com) wrote:
> Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto:
> >From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
> >
> >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 / dgilb...@redhat.com / Manchester, UK
