Re: [Qemu-devel] [PATCH v5 14/45] Move loadvm_handlers into MigrationIncomingState

[David Gibson]

On Tue, Mar 10, 2015 at 10:12:14AM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Wed, Feb 25, 2015 at 04:51:37PM +0000, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > 
> > > In postcopy we need the loadvm_handlers to be used in a couple
> > > of different instances of the loadvm loop/routine, and thus
> > > it can't be local any more.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > > ---
> > >  include/migration/migration.h |  5 +++++
> > >  include/migration/vmstate.h   |  2 ++
> > >  include/qemu/typedefs.h       |  1 +
> > >  migration/migration.c         |  2 ++
> > >  savevm.c                      | 28 ++++++++++++++++------------
> > >  5 files changed, 26 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > > index 3776e86..751caa0 100644
> > > --- a/include/migration/migration.h
> > > +++ b/include/migration/migration.h
> > > @@ -50,10 +50,15 @@ enum mig_rpcomm_cmd {
> > >  
> > >  typedef struct MigrationState MigrationState;
> > >  
> > > +typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head;
> > > +
> > >  /* State for the incoming migration */
> > >  struct MigrationIncomingState {
> > >      QEMUFile *file;
> > >  
> > > +    /* See savevm.c */
> > > +    LoadStateEntry_Head loadvm_handlers;
> > > +
> > >      QEMUFile *return_path;
> > >      QemuMutex      rp_mutex;    /* We send replies from multiple threads 
> > > */
> > >  };
> > > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > index c20f2d1..18da207 100644
> > > --- a/include/migration/vmstate.h
> > > +++ b/include/migration/vmstate.h
> > > @@ -797,6 +797,8 @@ extern const VMStateInfo vmstate_info_bitmap;
> > >  
> > >  #define SELF_ANNOUNCE_ROUNDS 5
> > >  
> > > +void loadvm_free_handlers(MigrationIncomingState *mis);
> > > +
> > >  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > >                         void *opaque, int version_id);
> > >  void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > > index 74dfad3..6fdcbcd 100644
> > > --- a/include/qemu/typedefs.h
> > > +++ b/include/qemu/typedefs.h
> > > @@ -31,6 +31,7 @@ typedef struct I2CBus I2CBus;
> > >  typedef struct I2SCodec I2SCodec;
> > >  typedef struct ISABus ISABus;
> > >  typedef struct ISADevice ISADevice;
> > > +typedef struct LoadStateEntry LoadStateEntry;
> > >  typedef struct MACAddr MACAddr;
> > >  typedef struct MachineClass MachineClass;
> > >  typedef struct MachineState MachineState;
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 34cd4fe..4592060 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -78,6 +78,7 @@ MigrationIncomingState 
> > > *migration_incoming_state_new(QEMUFile* f)
> > >  {
> > >      mis_current = g_malloc0(sizeof(MigrationIncomingState));
> > >      mis_current->file = f;
> > > +    QLIST_INIT(&mis_current->loadvm_handlers);
> > >      qemu_mutex_init(&mis_current->rp_mutex);
> > >  
> > >      return mis_current;
> > > @@ -85,6 +86,7 @@ MigrationIncomingState 
> > > *migration_incoming_state_new(QEMUFile* f)
> > >  
> > >  void migration_incoming_state_destroy(void)
> > >  {
> > > +    loadvm_free_handlers(mis_current);
> > 
> > AFAICT this is the only caler of loadvm_free_handlers(), so why not
> > just open-code it here?
> 
> I was keeping this handler list as owned by savevm.c; all it's allocation
> and it's use are done in savevm.c, so it would seem odd to open code
> freeing the list in a separate file.
> 
> > >      g_free(mis_current);
> > >      mis_current = NULL;
> > >  }
> > > diff --git a/savevm.c b/savevm.c
> > > index 7084d07..f42713d 100644
> > > --- a/savevm.c
> > > +++ b/savevm.c
> > > @@ -1019,18 +1019,26 @@ static int loadvm_process_command(QEMUFile *f)
> > >      return 0;
> > >  }
> > >  
> > > -typedef struct LoadStateEntry {
> > > +struct LoadStateEntry {
> > 
> > Why remove the typedef?
> 
> Because it is now typedef'd in typedefs.h, and older gcc (RHEL6)
> object to two typedefs even if they're of the same thing.

Ok, makes sense.

Reviewed-by: David Gibson <address@hidden>

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

pgphKigNfJQ9G.pgp
Description: PGP signature