Re: [Qemu-devel] [PATCH V5 2/8] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2013-7-18 19:48, Kevin Wolf 写道:
> 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 <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina <phrdina@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.
>
   will remove.

>> --- 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
>


-- 
Best Regards

Wenchao Xia