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 <phrd...@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