From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54600) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVH9L-0001nw-Du for qemu-devel@nongnu.org; Tue, 10 Mar 2015 06:12:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVH9J-00014d-Qb for qemu-devel@nongnu.org; Tue, 10 Mar 2015 06:12:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52040) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVH9J-00013e-Ig for qemu-devel@nongnu.org; Tue, 10 Mar 2015 06:12:29 -0400 Date: Tue, 10 Mar 2015 10:12:14 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20150310101213.GC2338@work-vm> References: <1424883128-9841-1-git-send-email-dgilbert@redhat.com> <1424883128-9841-15-git-send-email-dgilbert@redhat.com> <20150310061912.GG11973@voom.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150310061912.GG11973@voom.redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 14/45] Move loadvm_handlers into MigrationIncomingState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com, pbonzini@redhat.com, yanghy@cn.fujitsu.com * David Gibson (david@gibson.dropbear.id.au) wrote: > On Wed, Feb 25, 2015 at 04:51:37PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > 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 > > --- > > 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. Dave > > QLIST_ENTRY(LoadStateEntry) entry; > > SaveStateEntry *se; > > int section_id; > > int version_id; > > -} LoadStateEntry; > > +}; > > > > -int qemu_loadvm_state(QEMUFile *f) > > +void loadvm_free_handlers(MigrationIncomingState *mis) > > { > > - QLIST_HEAD(, LoadStateEntry) loadvm_handlers = > > - QLIST_HEAD_INITIALIZER(loadvm_handlers); > > LoadStateEntry *le, *new_le; > > + > > + QLIST_FOREACH_SAFE(le, &mis->loadvm_handlers, entry, new_le) { > > + QLIST_REMOVE(le, entry); > > + g_free(le); > > + } > > +} > > + > > +int qemu_loadvm_state(QEMUFile *f) > > +{ > > + MigrationIncomingState *mis = migration_incoming_get_current(); > > Error *local_err = NULL; > > uint8_t section_type; > > unsigned int v; > > @@ -1061,6 +1069,7 @@ int qemu_loadvm_state(QEMUFile *f) > > while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) { > > uint32_t instance_id, version_id, section_id; > > SaveStateEntry *se; > > + LoadStateEntry *le; > > char idstr[256]; > > > > trace_qemu_loadvm_state_section(section_type); > > @@ -1102,7 +1111,7 @@ int qemu_loadvm_state(QEMUFile *f) > > le->se = se; > > le->section_id = section_id; > > le->version_id = version_id; > > - QLIST_INSERT_HEAD(&loadvm_handlers, le, entry); > > + QLIST_INSERT_HEAD(&mis->loadvm_handlers, le, entry); > > > > ret = vmstate_load(f, le->se, le->version_id); > > if (ret < 0) { > > @@ -1116,7 +1125,7 @@ int qemu_loadvm_state(QEMUFile *f) > > section_id = qemu_get_be32(f); > > > > trace_qemu_loadvm_state_section_partend(section_id); > > - QLIST_FOREACH(le, &loadvm_handlers, entry) { > > + QLIST_FOREACH(le, &mis->loadvm_handlers, entry) { > > if (le->section_id == section_id) { > > break; > > } > > @@ -1152,11 +1161,6 @@ int qemu_loadvm_state(QEMUFile *f) > > ret = 0; > > > > out: > > - QLIST_FOREACH_SAFE(le, &loadvm_handlers, entry, new_le) { > > - QLIST_REMOVE(le, entry); > > - g_free(le); > > - } > > - > > if (ret == 0) { > > ret = qemu_file_get_error(f); > > } > > -- > 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 / dgilbert@redhat.com / Manchester, UK