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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2976 bytes --]

On Wed, Feb 25, 2015 at 04:51:30PM +0000, 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.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/migration/qemu-file.h  |  7 +++++
>  migration/qemu-file-internal.h |  2 ++
>  migration/qemu-file-unix.c     | 58 +++++++++++++++++++++++++++++++++++-------
>  migration/qemu-file.c          | 12 +++++++++
>  4 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 6ae0b03..3c38963 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);
>  int 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/migration/qemu-file-internal.h b/migration/qemu-file-internal.h
> index d95e853..a39b8e3 100644
> --- a/migration/qemu-file-internal.h
> +++ b/migration/qemu-file-internal.h
> @@ -48,6 +48,8 @@ struct QEMUFile {
>      unsigned int iovcnt;
>  
>      int last_error;
> +
> +    struct QEMUFile *return_path;

AFAICT, the only thing this field is used for is an assert, which
seems a bit pointless.  I'd suggest either getting rid of it, or
make qemu_file_get_return_path() safely idempotent by having it only
call the FileOps pointer if QEMUFile::return_path is non-NULL,
otherwise just return the existing return_path.

Setting the field probably belongs better in the wrapper than in the
socket specific callback, too, since there's nothing inherently
related to the socket implementation about it.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]