From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58021) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuJO6-0003WR-St for qemu-devel@nongnu.org; Mon, 18 May 2015 07:39:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuJO3-0006il-JQ for qemu-devel@nongnu.org; Mon, 18 May 2015 07:39:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51938) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuJO3-0006ic-Cg for qemu-devel@nongnu.org; Mon, 18 May 2015 07:39:11 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4IBdAHv027674 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 18 May 2015 07:39:10 -0400 Date: Mon, 18 May 2015 12:39:06 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20150518113905.GH2201@work-vm> References: <1431620920-19710-1-git-send-email-quintela@redhat.com> <1431620920-19710-10-git-send-email-quintela@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1431620920-19710-10-git-send-email-quintela@redhat.com> Subject: Re: [Qemu-devel] [PATCH 9/9] migration: Add configuration section List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org * Juan Quintela (quintela@redhat.com) wrote: > It needs to be the first one and it is not optional, that is the reason > why it is opencoded. For new machine types, it is required than machine > type name is the same in both sides. > > It is just done right now for pc's. > > Signed-off-by: Juan Quintela > --- > hw/i386/pc_piix.c | 1 + > hw/i386/pc_q35.c | 1 + > include/migration/migration.h | 2 ++ > savevm.c | 72 ++++++++++++++++++++++++++++++++++++++++--- > 4 files changed, 71 insertions(+), 5 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 5c04784..95806b3 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -314,6 +314,7 @@ static void pc_init_pci(MachineState *machine) > static void pc_compat_2_3(MachineState *machine) > { > global_state_set_optional(); > + savevm_skip_configuration(); > } > > static void pc_compat_2_2(MachineState *machine) > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index cc5827a..e32c040 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -293,6 +293,7 @@ static void pc_q35_init(MachineState *machine) > static void pc_compat_2_3(MachineState *machine) > { > global_state_set_optional(); > + savevm_skip_configuration(); It's a shame that these two functions, that do basically the same thing (to two different pieces of data) have such different names. > } > > static void pc_compat_2_2(MachineState *machine) > diff --git a/include/migration/migration.h b/include/migration/migration.h > index f939d88..da89827 100644 > --- a/include/migration/migration.h > +++ b/include/migration/migration.h > @@ -34,6 +34,7 @@ > #define QEMU_VM_SECTION_FULL 0x04 > #define QEMU_VM_SUBSECTION 0x05 > #define QEMU_VM_VMDESCRIPTION 0x06 > +#define QEMU_VM_CONFIGURATION 0x07 > > struct MigrationParams { > bool blk; > @@ -184,4 +185,5 @@ void register_global_state(void); > void global_state_store(void); > char *global_state_get_runstate(void); > void global_state_set_optional(void); > +void savevm_skip_configuration(void); > #endif > diff --git a/savevm.c b/savevm.c > index 2b4e554..ea149e7 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -238,11 +238,55 @@ typedef struct SaveStateEntry { > typedef struct SaveState { > QTAILQ_HEAD(, SaveStateEntry) handlers; > int global_section_id; > + bool skip_configuration; > + uint32_t len; > + char *name; > } SaveState; > > static SaveState savevm_state = { > .handlers = QTAILQ_HEAD_INITIALIZER(savevm_state.handlers), > .global_section_id = 0, > + .skip_configuration = false, > +}; > + > +void savevm_skip_configuration(void) > +{ > + savevm_state.skip_configuration = true; > +} > + > + > +static void configuration_pre_save(void *opaque) > +{ > + SaveState *state = opaque; > + const char *current_name = MACHINE_GET_CLASS(current_machine)->name; > + > + state->len = strlen(current_name); > + state->name = strdup(current_name); Will that ever get freed? If it never gets freed is it safe to just make it state->len = current_name; > + > +static int configuration_post_load(void *opaque, int version_id) > +{ > + SaveState *state = opaque; > + const char *current_name = MACHINE_GET_CLASS(current_machine)->name; > + > + if (strncmp(state->name, current_name, state->len) != 0) { > + error_report("Machine type received is '%s' and local is '%s'", > + state->name, current_name); > + return -EINVAL; > + } > + return 0; > +} > + > +static const VMStateDescription vmstate_configuration = { > + .name = "configuartion", Typo! configuration Dave > + .version_id = 1, > + .post_load = configuration_post_load, > + .pre_save = configuration_pre_save, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(len, SaveState), > + VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, 0, len), > + VMSTATE_END_OF_LIST() > + }, > }; > > static void dump_vmstate_vmsd(FILE *out_file, > @@ -625,7 +669,7 @@ void qemu_savevm_state_begin(QEMUFile *f, > const MigrationParams *params) > { > SaveStateEntry *se; > - int ret; > + int ret, len; > > trace_savevm_state_begin(); > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > @@ -638,9 +682,13 @@ void qemu_savevm_state_begin(QEMUFile *f, > qemu_put_be32(f, QEMU_VM_FILE_MAGIC); > qemu_put_be32(f, QEMU_VM_FILE_VERSION); > > - QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > - int len; > + /* We want this to be the 1st section on the migration stream */ > + if (!savevm_state.skip_configuration) { > + qemu_put_byte(f, QEMU_VM_CONFIGURATION); > + vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0); > + } > > + QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > if (!se->ops || !se->ops->save_live_setup) { > continue; > } > @@ -946,7 +994,7 @@ int qemu_loadvm_state(QEMUFile *f) > Error *local_err = NULL; > uint8_t section_type; > unsigned int v; > - int ret; > + int ret, len; > int file_error_after_eof = -1; > > if (qemu_savevm_state_blocked(&local_err)) { > @@ -970,11 +1018,25 @@ int qemu_loadvm_state(QEMUFile *f) > return -ENOTSUP; > } > > + /* if it exist, we want the configuration to be the 1st section > + and we require it to be there. That is the reason why we don't > + read it on the while loop. */ > + if (!savevm_state.skip_configuration) { > + if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) { > + error_report("Configuration section missing"); > + return -EINVAL; > + } > + ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0); > + > + if (ret) { > + return ret; > + } > + } > + > while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) { > uint32_t instance_id, version_id, section_id; > SaveStateEntry *se; > char idstr[257]; > - int len; > > trace_qemu_loadvm_state_section(section_type); > switch (section_type) { > -- > 2.4.0 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK