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

[Kevin Wolf]

Am 18.04.2013 um 15:19 hat Pavel Hrdina geschrieben:
> On 18.4.2013 14:55, Kevin Wolf wrote:
> >Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> >>--- 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.

I don't think it's the right approach to let the lowest layer add all
information that is potentially useful in some contexts. It should only
consider what information is essential for its direct caller.

You are right that in delvm multiple devices can be affected. This is
why delvm should amend the error message with any information that is
required for the error message to be helpful in the context of delvm.
This is essentially what I suggested here:

> >>@@ -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));

See how I added the device name and the context of the error ("while
deleteing the old snapshot") only in the place where it's actually
necessary for the caller to understand the error?

By the way, I'm also unsure if the "qcow2:" or "rbd:" prefix is actually
helpful.

Kevin