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

[Pavel Hrdina]

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