Re: [Qemu-devel] [PATCH COLO-Frame v12 30/38] savevm: Split load vm state function qemu_loadvm_state

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

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 <zhang.zhanghailiang@huawei.com>
>> ---
>>   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 <dgilbert@redhat.com>
>

Thanks.

>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>