[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm |
Date: |
Fri, 3 May 2013 12:50:18 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm,
Kevin Wolf <=