Re: [Qemu-devel] [PATCH v2 11/12] qapi: Convert savevm

[Kevin Wolf <kwolf@redhat.com>]

Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-save takes one parameter name and the name is
> mandatory. The command returns SnapshotInfo on success, otherwise it returns
> an error message. If there is a snapshot with the same name it also returns
> an error message and if you want to overwrite that snapshot, you will have to
> at first call vm-snapshot-delete.
> 
> HMP command savevm now has one optional parameter name and one flag -f.
> If the name parameter isn't provided the HMP command will create new one for
> internally used vm-snapshot-save. You can also specify the -f flag for overwrite
> existing snapshot which will internally use vm-snapshot-delete before
> vm-snapshot-save, otherwise it will print an error message if there is already
> a snapshot with the same name. If something else goes wrong, an error message
> will be printed.
> 
> These improves behavior of the command to let you select only the name of the
> snapshot you want to create. This will ensure that if you want snapshot with
> the name '2', it will not rewrite or fail if there is any snapshot with id '2'.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

> --- a/hmp.c
> +++ b/hmp.c
> @@ -1491,3 +1491,52 @@ void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
>      qapi_free_SnapshotInfo(info);
>      hmp_handle_error(mon, &local_err);
>  }
> +
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    bool force = qdict_get_try_bool(qdict, "force", 0);
> +    Error *local_err = NULL;
> +    SnapshotInfo *info = NULL;
> +    qemu_timeval tv;
> +    struct tm tm;
> +    char tmp_name[256];
> +
> +    if (!name) {
> +        localtime_r((const time_t *)&tv.tv_sec, &tm);
> +        strftime(tmp_name, sizeof(tmp_name), "vm-%Y%m%d%H%M%S", &tm);
> +        name = tmp_name;
> +    }
> +
> +    if (force) {
> +        info = qmp_vm_snapshot_delete(true, name, false, NULL, &local_err);
> +        /* We don't need print info about deleted snapshot. It still needs to
> +         * be freed. */
> +        qapi_free_SnapshotInfo(info);
> +        if (error_is_set(&local_err)) {
> +            hmp_handle_error(mon, &local_err);
> +            return;
> +        }
> +    }

Deleting a snapshot that doesn't exist returns an error. This means that
you can use -f _only_ to overwrite a snapshot. I think this is
unexpected, with -f all cases that work without -f should keep working.

> +
> +    info = qmp_vm_snapshot_save(name, &local_err);
> +
> +    if (info) {
> +        char buf[256];
> +        QEMUSnapshotInfo sn = {
> +            .vm_state_size = info->vm_state_size,
> +            .date_sec = info->date_sec,
> +            .date_nsec = info->date_nsec,
> +            .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
> +                             info->vm_clock_nsec,
> +        };
> +        pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
> +        pstrcpy(sn.name, sizeof(sn.name), info->name);
> +        monitor_printf(mon, "Created snapshot's info:\n");
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
> +    }
> +
> +    qapi_free_SnapshotInfo(info);
> +    hmp_handle_error(mon, &local_err);
> +}

> --- a/savevm.c
> +++ b/savevm.c
> @@ -2398,36 +2369,24 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      sn->date_nsec = tv.tv_usec * 1000;
>      sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>  
> -    if (name) {
> -        if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, true)) {
> -            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> -            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> -        } else {
> -            pstrcpy(sn->name, sizeof(sn->name), name);
> -        }
> -    } else {
> -        /* cast below needed for OpenBSD where tv_sec is still 'long' */
> -        localtime_r((const time_t *)&tv.tv_sec, &tm);
> -        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
> -    }
> -
> -    /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(mon, name) < 0) {
> +    if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL, false)) {
> +        error_setg(errp, "Snapshot '%s' exists", name);

This is only checked for the VM state device. For all other devices the
case isn't handled any more.

>          goto the_end;
> +    } else {
> +        pstrcpy(sn->name, sizeof(sn->name), name);
>      }
>  
>      /* save the VM state */
>      f = qemu_fopen_bdrv(bs, 1);
>      if (!f) {
> -        monitor_printf(mon, "Could not open VM state file\n");
> +        error_setg(errp, "Failed to open '%s' file", bdrv_get_device_name(bs));
>          goto the_end;
>      }
>      qemu_savevm_state(f, &local_err);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
>      if (error_is_set(&local_err)) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> +        error_propagate(errp, local_err);
>          goto the_end;
>      }
>  
> @@ -2440,18 +2399,29 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>              sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>              bdrv_snapshot_create(bs1, sn, &local_err);
>              if (error_is_set(&local_err)) {
> -                qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                              "Failed to create snapshot for device '%s': %s",
> -                              bdrv_get_device_name(bs1),
> -                              error_get_pretty(local_err));
> +                error_setg(errp, "Failed to create snapshot for "
> +                           "device '%s': %s", bdrv_get_device_name(bs1),
> +                           error_get_pretty(local_err));
>                  error_free(local_err);
> +                goto the_end;
>              }
>          }
>      }
>  
> +    info = g_malloc0(sizeof(SnapshotInfo));
> +    info->id = g_strdup(sn->id_str);
> +    info->name = g_strdup(sn->name);
> +    info->date_nsec = sn->date_nsec;
> +    info->date_sec = sn->date_sec;
> +    info->vm_state_size = vm_state_size;
> +    info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000;
> +    info->vm_clock_sec = sn->vm_clock_nsec / 1000000000;

You could consider using a compound literal for better readability:

info = g_malloc(...);
*info = (SnapshotInfo) {
    .id         = g_strdup(sn->id_str),
    .name       = g_strdup(sn->name),
    .date_nsec  = sn->date_nsec,
    ...
};

>   the_end:
>      if (saved_vm_running)
>          vm_start();
> +
> +    return info;
>  }

Kevin