From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54316) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aCM0y-0006eC-Si for qemu-devel@nongnu.org; Fri, 25 Dec 2015 01:38:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aCM0v-0008F3-MA for qemu-devel@nongnu.org; Fri, 25 Dec 2015 01:38:12 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:64113) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aCM0u-0008DV-Pz for qemu-devel@nongnu.org; Fri, 25 Dec 2015 01:38:09 -0500 References: <1450167779-9960-1-git-send-email-zhang.zhanghailiang@huawei.com> <1450167779-9960-31-git-send-email-zhang.zhanghailiang@huawei.com> <20151215120814.GF2500@work-vm> From: Hailiang Zhang Message-ID: <567CE435.8050206@huawei.com> Date: Fri, 25 Dec 2015 14:37:41 +0800 MIME-Version: 1.0 In-Reply-To: <20151215120814.GF2500@work-vm> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v12 30/38] savevm: Split load vm state function qemu_loadvm_state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: lizhijian@cn.fujitsu.com, quintela@redhat.com, yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, qemu-devel@nongnu.org, arei.gonglei@huawei.com, stefanha@redhat.com, amit.shah@redhat.com, hongyang.yang@easystack.cn On 2015/12/15 20:08, Dr. David Alan Gilbert wrote: > * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote: >> qemu_loadvm_state is too long, and we can simplify it by splitting up >> with three helper functions. > > Yes, good idea. > >> Signed-off-by: zhanghailiang >> --- >> migration/savevm.c | 161 ++++++++++++++++++++++++++++++++--------------------- >> 1 file changed, 97 insertions(+), 64 deletions(-) >> >> diff --git a/migration/savevm.c b/migration/savevm.c >> index f102870..c7c26d8 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -1710,90 +1710,123 @@ void loadvm_free_handlers(MigrationIncomingState *mis) >> } >> } >> >> +static int >> +qemu_loadvm_section_start_full(QEMUFile *f, MigrationIncomingState *mis) >> +{ >> + uint32_t instance_id, version_id, section_id; >> + SaveStateEntry *se; >> + LoadStateEntry *le; >> + char idstr[256]; >> + int ret; >> + >> + /* Read section start */ >> + section_id = qemu_get_be32(f); >> + if (!qemu_get_counted_string(f, idstr)) { >> + error_report("Unable to read ID string for section %u", >> + section_id); >> + return -EINVAL; >> + } >> + instance_id = qemu_get_be32(f); >> + version_id = qemu_get_be32(f); >> + >> + trace_qemu_loadvm_state_section_startfull(section_id, idstr, >> + instance_id, version_id); >> + /* Find savevm section */ >> + se = find_se(idstr, instance_id); >> + if (se == NULL) { >> + error_report("Unknown savevm section or instance '%s' %d", >> + idstr, instance_id); >> + ret = -EINVAL; >> + return ret; > > Minor; you don't need 'ret' there, just return -EINVAL. > >> + } >> + >> + /* Validate version */ >> + if (version_id > se->version_id) { >> + error_report("savevm: unsupported version %d for '%s' v%d", >> + version_id, idstr, se->version_id); >> + ret = -EINVAL; >> + return ret; > > same > >> + } >> + >> + /* Add entry */ >> + le = g_malloc0(sizeof(*le)); >> + >> + le->se = se; >> + le->section_id = section_id; >> + le->version_id = version_id; >> + QLIST_INSERT_HEAD(&mis->loadvm_handlers, le, entry); >> + >> + ret = vmstate_load(f, le->se, le->version_id); >> + if (ret < 0) { >> + error_report("error while loading state for instance 0x%x of" >> + " device '%s'", instance_id, idstr); >> + return ret; >> + } >> + if (!check_section_footer(f, le)) { >> + ret = -EINVAL; >> + return ret; > > same. > >> + } >> + >> + return 0; >> +} >> + >> +static int >> +qemu_loadvm_section_part_end(QEMUFile *f, MigrationIncomingState *mis) >> +{ >> + uint32_t section_id; >> + LoadStateEntry *le; >> + int ret; >> + >> + section_id = qemu_get_be32(f); >> + >> + trace_qemu_loadvm_state_section_partend(section_id); >> + QLIST_FOREACH(le, &mis->loadvm_handlers, entry) { >> + if (le->section_id == section_id) { >> + break; >> + } >> + } >> + if (le == NULL) { >> + error_report("Unknown savevm section %d", section_id); >> + ret = -EINVAL; >> + return ret; > > same > >> + } >> + >> + ret = vmstate_load(f, le->se, le->version_id); >> + if (ret < 0) { >> + error_report("error while loading state section id %d(%s)", >> + section_id, le->se->idstr); >> + return ret; >> + } >> + if (!check_section_footer(f, le)) { >> + ret = -EINVAL; >> + return ret; > > same > >> + } >> + >> + return 0; >> +} >> + >> static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis) >> { >> uint8_t section_type; >> int ret; >> >> 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); >> switch (section_type) { >> case QEMU_VM_SECTION_START: >> case QEMU_VM_SECTION_FULL: >> - /* Read section start */ >> - section_id = qemu_get_be32(f); >> - if (!qemu_get_counted_string(f, idstr)) { >> - error_report("Unable to read ID string for section %u", >> - section_id); >> - return -EINVAL; >> - } >> - instance_id = qemu_get_be32(f); >> - version_id = qemu_get_be32(f); >> - >> - trace_qemu_loadvm_state_section_startfull(section_id, idstr, >> - instance_id, version_id); >> - /* Find savevm section */ >> - se = find_se(idstr, instance_id); >> - if (se == NULL) { >> - error_report("Unknown savevm section or instance '%s' %d", >> - idstr, instance_id); >> - return -EINVAL; >> - } >> - >> - /* Validate version */ >> - if (version_id > se->version_id) { >> - error_report("savevm: unsupported version %d for '%s' v%d", >> - version_id, idstr, se->version_id); >> - return -EINVAL; >> - } >> - >> - /* Add entry */ >> - le = g_malloc0(sizeof(*le)); >> - >> - le->se = se; >> - le->section_id = section_id; >> - le->version_id = version_id; >> - QLIST_INSERT_HEAD(&mis->loadvm_handlers, le, entry); >> - >> - ret = vmstate_load(f, le->se, le->version_id); >> + ret = qemu_loadvm_section_start_full(f, mis); >> if (ret < 0) { >> - error_report("error while loading state for instance 0x%x of" >> - " device '%s'", instance_id, idstr); >> return ret; >> } >> - if (!check_section_footer(f, le)) { >> - return -EINVAL; >> - } >> break; >> case QEMU_VM_SECTION_PART: >> case QEMU_VM_SECTION_END: >> - section_id = qemu_get_be32(f); >> - >> - trace_qemu_loadvm_state_section_partend(section_id); >> - QLIST_FOREACH(le, &mis->loadvm_handlers, entry) { >> - if (le->section_id == section_id) { >> - break; >> - } >> - } >> - if (le == NULL) { >> - error_report("Unknown savevm section %d", section_id); >> - return -EINVAL; >> - } >> - >> - ret = vmstate_load(f, le->se, le->version_id); >> + ret = qemu_loadvm_section_part_end(f, mis); >> if (ret < 0) { >> - error_report("error while loading state section id %d(%s)", >> - section_id, le->se->idstr); >> return ret; >> } >> - if (!check_section_footer(f, le)) { >> - return -EINVAL; >> - } >> break; >> case QEMU_VM_COMMAND: >> ret = loadvm_process_command(f); >> -- >> 1.8.3.1 > > > Other than the minor return fixups; I will fix them all in next version. > Reviewed-by: Dr. David Alan Gilbert > Thanks. >> >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > . >