Re: [Qemu-devel] [PATCH 02/11] block: update error reporting for bdrv_snapshot_delete() and related functions

[Pavel Hrdina <phrdina@redhat.com>]

On 18.4.2013 14:55, Kevin Wolf wrote:
> Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
>>
>>       /*
>> @@ -567,14 +573,18 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>>       ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
>>                                            sn.l1_size, -1);
>>       if (ret < 0) {
>> -        return ret;
>> +        error_setg_errno(errp, -ret,
>> +                         "qcow2: failed to update snapshot refcount");
>
> I'd make it "qcow2: Failed to update refcounts". It's the refcounts of
> all clusters referred to by the snapshot's L1 table, not of the snapshot
> itself.

Thanks for correction. I'm not that much familiar with qcow2 internals.

>
>> +        return;
>>       }
>>       qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
>>
>>       /* must update the copied flag on the current cluster offsets */
>>       ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
>>       if (ret < 0) {
>> -        return ret;
>> +        error_setg_errno(errp, -ret,
>> +                         "qcow2: failed to update snapshot refcount");
>
> "qcow2: Failed to update cluster flags"

Again thanks for correction.

>
>> +        return;
>>       }
>>
>>   #ifdef DEBUG_ALLOC
>> @@ -583,7 +593,6 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>>           qcow2_check_refcounts(bs, &result, 0);
>>       }
>>   #endif
>> -    return 0;
>>   }
>>
>>   int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 9421843..dbd332d 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -384,7 +384,9 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
>>   /* qcow2-snapshot.c functions */
>>   int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
>>   int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
>> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
>> +void qcow2_snapshot_delete(BlockDriverState *bs,
>> +                           const char *snapshot_id,
>> +                           Error **errp);
>>   int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
>>   int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
>>
>> diff --git a/block/rbd.c b/block/rbd.c
>> index 141b488..c10edbf 100644
>> --- a/block/rbd.c
>> +++ b/block/rbd.c
>> @@ -871,14 +871,19 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
>>       return 0;
>>   }
>>
>> -static int qemu_rbd_snap_remove(BlockDriverState *bs,
>> -                                const char *snapshot_name)
>> +static void qemu_rbd_snap_remove(BlockDriverState *bs,
>> +                                 const char *snapshot_name,
>> +                                 Error **errp)
>>   {
>>       BDRVRBDState *s = bs->opaque;
>>       int r;
>>
>>       r = rbd_snap_remove(s->image, snapshot_name);
>> -    return r;
>> +    if (r < 0) {
>> +        error_setg_errno(errp, -r, "rbd: failed to remove snapshot '%s' on "
>> +                         "device '%s'", snapshot_name,
>> +                         bdrv_get_device_name(bs));
>
> Remove the device name. You didn't have it in the qcow2 errors either.

Or maybe I should also add the device name in the qcow2 errors, because 
as Eric write to you these function are used also for vm-snapshot-delete 
and devlm and knowing which device failed is important.

>
>> +    }
>>   }
>>
>>   static int qemu_rbd_snap_rollback(BlockDriverState *bs,
>> diff --git a/block/sheepdog.c b/block/sheepdog.c
>> index 1c5b532..270fa64 100644
>> --- a/block/sheepdog.c
>> +++ b/block/sheepdog.c
>> @@ -1908,10 +1908,12 @@ out:
>>       return ret;
>>   }
>>
---
>> diff --git a/savevm.c b/savevm.c
>> index 53515cb..6af84fd 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2225,18 +2225,17 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>>   {
>>       BlockDriverState *bs;
>>       QEMUSnapshotInfo sn1, *snapshot = &sn1;
>> -    int ret;
>> +    Error *local_err = NULL;
>>
>>       bs = NULL;
>>       while ((bs = bdrv_next(bs))) {
>>           if (bdrv_can_snapshot(bs) &&
>>               bdrv_snapshot_find(bs, snapshot, name) >= 0)
>>           {
>> -            ret = bdrv_snapshot_delete(bs, name);
>> -            if (ret < 0) {
>> -                monitor_printf(mon,
>> -                               "Error while deleting snapshot on '%s'\n",
>> -                               bdrv_get_device_name(bs));
>> +            bdrv_snapshot_delete(bs, name, &local_err);
>> +            if (error_is_set(&local_err)) {
>> +                monitor_printf(mon, "%s\n", error_get_pretty(local_err));
>
> Here the additional monitor_printf() actually had meaningful additional
> information. Deleting an old snapshot is an implicitly taken action and
> not explicitly requested, so an error message should indicate that it
> happened during the deletion. Maybe something like:

Function del_existing_snapshots will be anyway dropped later in patch 
series so this has no actual value. But I should probably make something 
similar to this for HMP command savevm.

Thanks

>
> qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                "Error while deleting old snapshot on device '%s': %s",
>                bdrv_get_device_name(bs), error_get_pretty(local_err));
>
>> +                error_free(local_err);
>>                   return -1;
>>               }
>>           }
>> @@ -2450,7 +2449,7 @@ int load_vmstate(const char *name)
>>   void do_delvm(Monitor *mon, const QDict *qdict)
>>   {
>>       BlockDriverState *bs, *bs1;
>> -    int ret;
>> +    Error *local_err = NULL;
>>       const char *name = qdict_get_str(qdict, "name");
>>
>>       bs = bdrv_snapshots();
>> @@ -2462,15 +2461,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);
>> -            if (ret < 0) {
>> -                if (ret == -ENOTSUP)
>> -                    monitor_printf(mon,
>> -                                   "Snapshots not supported on device '%s'\n",
>> -                                   bdrv_get_device_name(bs1));
>> -                else
>> -                    monitor_printf(mon, "Error %d while deleting snapshot on "
>> -                                   "'%s'\n", ret, bdrv_get_device_name(bs1));
>> +            bdrv_snapshot_delete(bs1, name, &local_err);
>> +            if (error_is_set(&local_err)) {
>> +                monitor_printf(mon, "%s\n", error_get_pretty(local_err));
>
> Either something like above, indicating the device name on which
> bdrv_snapshot_delete() failed, or qerror_report_err().

Like I wrote few lines above, I should add the device name into all 
errors also in qcow2, rbd and sheepdog. This also applies for 
bdrv_snapshot_goto/create/list.

Pavel

>
>> +                error_free(local_err);
>>               }
>>           }
>>       }
>
> Kevin
>