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

[Dr. David Alan Gilbert]

* David Gibson (address@hidden) wrote:
> On Wed, Feb 25, 2015 at 04:51:30PM +0000, Dr. David Alan Gilbert (git) wrote:
> > 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'.
> > 
> > Wire it up for 'socket' QEMUFile's using a dup'd fd.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  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

Done; it's gone.

Dave

> 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


--
Dr. David Alan Gilbert / address@hidden / Manchester, UK