Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm

[Kevin Wolf <kwolf@redhat.com>]

Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-delete takes two parameters: name and id. They are
> optional, but one of the name or id must be provided. If both are provided they
> will match only the snapshot with the same name and id. The command returns
> SnapshotInfo only if the snapshot exists, otherwise it returns an error message.
> 
> HMP command delvm now uses the new vm-snapshot-delete, but behave slightly
> different. The delvm takes one optional flag -i and one parameter name. If you
> provide only the name parameter, it will match only snapshots with that name.
> If you also provide the flag, it will match only snapshots with name as id.
> Information about successfully deleted snapshot will be printed, otherwise
> an error message will be printed.
> 
> These improves behavior of the command to be more strict on selecting snapshots
> because actual behavior is wrong. Now if you want to delete snapshot with name '2'
> but there is no snapshot with that name it could delete snapshot with id '2' and
> that isn't what you want.
> 
> There is also small hack to ensure that in each block device with different
> driver type the correct snapshot is deleted. The 'qcow2' and 'sheepdog' internally
> search at first for id but 'rbd' has only name and therefore search only for name.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>

One general comment: I'm not sure how much sense it really makes to
delete snapshots based on ID on multiple images. The user doesn't have
any influence on the ID, I think, so a VM-wide snapshot can only be
identified by name across multiple images.

> --- a/savevm.c
> +++ b/savevm.c
> @@ -40,6 +40,7 @@
>  #include "trace.h"
>  #include "qemu/bitops.h"
>  #include "qemu/iov.h"
> +#include "block/block_int.h"
>  
>  #define SELF_ANNOUNCE_ROUNDS 5
>  
> @@ -2556,31 +2557,73 @@ int load_vmstate(const char *name)
>      return 0;
>  }
>  
> -void do_delvm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> +                                     const bool has_id, const char *id,
> +                                     Error **errp)
>  {
> -    BlockDriverState *bs, *bs1;
> +    BlockDriverState *bs;
> +    SnapshotInfo *info = NULL;
> +    QEMUSnapshotInfo sn;
>      Error *local_err = NULL;
> -    const char *name = qdict_get_str(qdict, "name");
> +
> +    if (!has_name && !has_id) {
> +        error_setg(errp, "Name or id must be provided");
> +        return NULL;
> +    }
> +
> +    if (!has_name) {
> +        name = NULL;
> +    }
> +    if (!has_id) {
> +        id = NULL;
> +    }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device supports snapshots\n");
> -        return;
> +        error_setg(errp, "No block device supports snapshots");
> +        return NULL;
>      }
>  
> -    bs1 = NULL;
> -    while ((bs1 = bdrv_next(bs1))) {
> -        if (bdrv_can_snapshot(bs1)) {
> -            bdrv_snapshot_delete(bs1, name, &local_err);
> +    if (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }

Why is this necessary? The check didn't exist before.

> +
> +    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 = sn.vm_state_size;
> +    info->vm_clock_nsec = sn.vm_clock_nsec % 1000000000;
> +    info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
> +
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {
> +        if (bdrv_can_snapshot(bs) &&
> +            bdrv_snapshot_find(bs, &sn, name, id, NULL, false)) {

Aha, this is the reason: The command is underspecified and you change
some behaviour that isn't mentioned in the documentation. Before, the
command would return an error if a device that supports snapshots
doesn't have the specific snapshot. After the patch, it would silently
ignore the snapshot - except in bdrv_snapshots(), which is more or less
randomly selected.

Why is this an improvement?

Independent of what we decide here, the result should be added to the
QMP documentation.

> +            /* Small hack to ensure that proper snapshot is deleted for every
> +             * block driver. This will be fixed soon. */
> +            if (!strcmp(bs->drv->format_name, "rbd")) {
> +                bdrv_snapshot_delete(bs, sn.name, &local_err);
> +            } else {
> +                bdrv_snapshot_delete(bs, sn.id_str, &local_err);
> +            }
>              if (error_is_set(&local_err)) {
> -                qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
> -                              "old snapshot on device '%s': %s",
> -                              bdrv_get_device_name(bs),
> -                              error_get_pretty(local_err));
> +                error_setg(errp, "Failed to delete  old snapshot on "
> +                           "device '%s': %s", bdrv_get_device_name(bs),
> +                           error_get_pretty(local_err));
>                  error_free(local_err);
>              }

You can't use error_setg() multiple times on the same errp, the second
call would trigger an assertion failure. Should we immediately break
after an error?

>          }
>      }
> +
> +    if (error_is_set(errp)) {
> +        g_free(info);
> +        return NULL;
> +    }
> +
> +    return info;
>  }

Kevin