Am 11.07.2013 um 07:46 hat Wenchao Xia geschrieben:
> Snapshot creation actually already distinguish id and name since it take
> a structured parameter *sn, but delete can't. Later an accurate delete
> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
> so change its prototype. Also *errp is added to tip error, but return
> value is kepted to let caller check what kind of error happens. Existing
> caller for it are savevm, delvm and qemu-img, they are not impacted by
> check the return value and do the operations again.
>
> Before this patch:
> For qcow2, it search id first then name to find the one to delete.
> For rbd, it search name.
> For sheepdog, it does nothing.
>
> After this patch:
> For qcow2, logic is the same by call it twice in caller.
> For rbd, it always fails in delete with id, but still search for name
> in second try, no change for user.
>
> Some code for *errp is based on Pavel's patch.
>
> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrd...@redhat.com>
> ---
> block/qcow2-snapshot.c | 66
> ++++++++++++++++++++++++++++++++++-----------
> block/qcow2.h | 5 +++-
> block/rbd.c | 23 +++++++++++++++-
> block/sheepdog.c | 5 +++-
> block/snapshot.c | 36 ++++++++++++++++++++++--
> include/block/block_int.h | 5 +++-
> include/block/snapshot.h | 5 +++-
> include/qemu-common.h | 3 ++
> qemu-img.c | 5 +++-
> savevm.c | 10 +++++-
> 10 files changed, 136 insertions(+), 27 deletions(-)
> @@ -531,15 +547,23 @@ fail:
> return ret;
> }
>
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_delete(BlockDriverState *bs,
> + const char *snapshot_id,
> + const char *name,
> + Error **errp)
> {
> BDRVQcowState *s = bs->opaque;
> QCowSnapshot sn;
> int snapshot_index, ret;
> + const char *device = bdrv_get_device_name(bs);
>
> /* Search the snapshot */
> - snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> + snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
> if (snapshot_index < 0) {
> + error_setg(errp,
> + "Can't find a snapshot with ID '%s' and name '%s' "
> + "on device '%s'",
> + STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name),
> device);
At least the "on device '%s'" part should be removed from the error
messages. It is something the caller can add if there is any ambiguity.
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
> break;
>
> case SNAPSHOT_DELETE:
> - ret = bdrv_snapshot_delete(bs, snapshot_name);
> + ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
> + if (ret == -ENOENT || ret == -EINVAL) {
> + ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
> + }
> if (ret) {
> error_report("Could not delete snapshot '%s': %d (%s)",
> snapshot_name, ret, strerror(-ret));
Why don't you use the new error messages?
> diff --git a/savevm.c b/savevm.c
> index e0491e7..56afebb 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const
> char *name)
> if (bdrv_can_snapshot(bs) &&
> bdrv_snapshot_find(bs, snapshot, name) >= 0)
> {
> - ret = bdrv_snapshot_delete(bs, name);
> + ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
> + if (ret == -ENOENT || ret == -EINVAL) {
> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
> + }
> if (ret < 0) {
> monitor_printf(mon,
> "Error while deleting snapshot on '%s'\n",
> @@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
> bs1 = NULL;
> while ((bs1 = bdrv_next(bs1))) {
> if (bdrv_can_snapshot(bs1)) {
> - ret = bdrv_snapshot_delete(bs1, name);
> + ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
> + if (ret == -ENOENT || ret == -EINVAL) {
> + ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
> + }
> if (ret < 0) {
> if (ret == -ENOTSUP)
> monitor_printf(mon,
Same thing here.
Kevin
