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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 04/07/2014 19:41, Dr. David Alan Gilbert (git) ha scritto:
> 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 |  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 ||
>