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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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