From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55304) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8oP3-0008Pq-9h for qemu-devel@nongnu.org; Tue, 15 Dec 2015 07:08:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8oP0-0004RX-17 for qemu-devel@nongnu.org; Tue, 15 Dec 2015 07:08:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58585) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8oOz-0004RI-Hp for qemu-devel@nongnu.org; Tue, 15 Dec 2015 07:08:21 -0500 Date: Tue, 15 Dec 2015 12:08:15 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20151215120814.GF2500@work-vm> References: <1450167779-9960-1-git-send-email-zhang.zhanghailiang@huawei.com> <1450167779-9960-31-git-send-email-zhang.zhanghailiang@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450167779-9960-31-git-send-email-zhang.zhanghailiang@huawei.com> 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: zhanghailiang 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 * 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; Reviewed-by: Dr. David Alan Gilbert > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK