Patchwork [02/11] block: update error reporting for bdrv_snapshot_delete() and related functions

login
register
mail settings
Submitter Pavel Hrdina
Date April 16, 2013, 4:05 p.m.
Message ID <6e22db8528d4a801af11b37d42a350eab9633636.1366127809.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/237020/
State New
Headers show

Comments

Pavel Hrdina - April 16, 2013, 4:05 p.m.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c                   | 22 ++++++++++++++--------
 block/qcow2-snapshot.c    | 21 +++++++++++++++------
 block/qcow2.h             |  4 +++-
 block/rbd.c               | 11 ++++++++---
 block/sheepdog.c          |  6 ++++--
 include/block/block.h     |  4 +++-
 include/block/block_int.h |  4 +++-
 qemu-img.c                |  9 ++++-----
 savevm.c                  | 26 ++++++++++----------------
 9 files changed, 64 insertions(+), 43 deletions(-)
Eric Blake - April 16, 2013, 5:14 p.m.
On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 22 ++++++++++++++--------
>  block/qcow2-snapshot.c    | 21 +++++++++++++++------
>  block/qcow2.h             |  4 +++-
>  block/rbd.c               | 11 ++++++++---
>  block/sheepdog.c          |  6 ++++--
>  include/block/block.h     |  4 +++-
>  include/block/block_int.h |  4 +++-
>  qemu-img.c                |  9 ++++-----
>  savevm.c                  | 26 ++++++++++----------------
>  9 files changed, 64 insertions(+), 43 deletions(-)
> 

> @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>      /* Search the snapshot */
>      snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
>      if (snapshot_index < 0) {
> -        return -ENOENT;
> +        error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name",
> +                   snapshot_id);
> +        return;

I haven't looked if you changed this later in the series to have
find_snapshot_by_id_or_name take an errp parameter, at which point you
could refactor the error reporting to that point in the stack.  That
might be a nice followup, but this patch is okay as-is.

> +++ b/block/sheepdog.c
> @@ -1908,10 +1908,12 @@ out:
>      return ret;
>  }
>  
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> +                               const char *snapshot_id,
> +                               Error **errp)
>  {
>      /* FIXME: Delete specified snapshot id.  */
> -    return 0;
> +    return;

No semantic change; but a trailing return; in a void function is not
necessary, and an empty body would suffice.  On the other hand, claiming
success when we didn't delete anything feels wrong.  Should we change
this function to call error_setg() and warn the user that deletion is
not supported at this time?  If so, that's probably better done as a
separate commit.

As my findings are best addressed in other patches, I'm okay with this
patch as-is, and you can use:

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf - April 18, 2013, 12:55 p.m.
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 22 ++++++++++++++--------
>  block/qcow2-snapshot.c    | 21 +++++++++++++++------
>  block/qcow2.h             |  4 +++-
>  block/rbd.c               | 11 ++++++++---
>  block/sheepdog.c          |  6 ++++--
>  include/block/block.h     |  4 +++-
>  include/block/block_int.h |  4 +++-
>  qemu-img.c                |  9 ++++-----
>  savevm.c                  | 26 ++++++++++----------------
>  9 files changed, 64 insertions(+), 43 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4ad663d..fb065e6 100644
> --- a/block.c
> +++ b/block.c
> @@ -3391,16 +3391,22 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +void bdrv_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> -        return -ENOMEDIUM;
> -    if (drv->bdrv_snapshot_delete)
> -        return drv->bdrv_snapshot_delete(bs, snapshot_id);
> -    if (bs->file)
> -        return bdrv_snapshot_delete(bs->file, snapshot_id);
> -    return -ENOTSUP;
> +
> +    if (!drv) {
> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));

I don't think the device name is useful here. It's always the device
that the user has specified in the monitor command.

Also, please start error messages with a capital letter. (This applies
to the whole patch, and probably also other patches in this series)

> +    } else if (drv->bdrv_snapshot_delete) {
> +        drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
> +    } else if (bs->file) {
> +        bdrv_snapshot_delete(bs->file, snapshot_id, errp);
> +    } else {
> +        error_setg(errp, "snapshots are not supported on device '%s'",
> +                   bdrv_get_device_name(bs));

Same thing with the device name here.

> +    }
>  }
>  
>  int bdrv_snapshot_list(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 992a5c8..2ebeadc 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -530,7 +530,9 @@ fail:
>      return ret;
>  }
>  
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +void qcow2_snapshot_delete(BlockDriverState *bs,
> +                           const char *snapshot_id,
> +                           Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot sn;
> @@ -539,7 +541,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>      /* Search the snapshot */
>      snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
>      if (snapshot_index < 0) {
> -        return -ENOENT;
> +        error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name",
> +                   snapshot_id);
> +        return;
>      }
>      sn = s->snapshots[snapshot_index];
>  
> @@ -550,7 +554,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>      s->nb_snapshots--;
>      ret = qcow2_write_snapshots(bs);
>      if (ret < 0) {
> -        return ret;
> +        error_setg_errno(errp, -ret, "qcow2: failed to remove snapshot '%s' "
> +                         "from snapshot list", sn.name);
> +        return;
>      }
>  
>      /*
> @@ -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.

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

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

> +    }
>  }
>  
>  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;
>  }
>  
> -static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +static void sd_snapshot_delete(BlockDriverState *bs,
> +                               const char *snapshot_id,
> +                               Error **errp)
>  {
>      /* FIXME: Delete specified snapshot id.  */
> -    return 0;
> +    return;
>  }

Looks like there should be some error_setg("Deleting snapshots not
supported yet by sheepdog"), possibly in a separate patch.

>  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
> diff --git a/include/block/block.h b/include/block/block.h
> index ebd9512..db8c6aa 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -337,7 +337,9 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info);
>  int bdrv_snapshot_goto(BlockDriverState *bs,
>                         const char *snapshot_id);
> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
> +void bdrv_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          Error **errp);
>  int bdrv_snapshot_list(BlockDriverState *bs,
>                         QEMUSnapshotInfo **psn_info);
>  int bdrv_snapshot_load_tmp(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 458cde3..53c52bb 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -157,7 +157,9 @@ struct BlockDriver {
>                                  QEMUSnapshotInfo *sn_info);
>      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
>                                const char *snapshot_id);
> -    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
> +    void (*bdrv_snapshot_delete)(BlockDriverState *bs,
> +                                 const char *snapshot_id,
> +                                 Error **errp);
>      int (*bdrv_snapshot_list)(BlockDriverState *bs,
>                                QEMUSnapshotInfo **psn_info);
>      int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
> diff --git a/qemu-img.c b/qemu-img.c
> index dbacdb7..4828fe4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1936,6 +1936,7 @@ static int img_snapshot(int argc, char **argv)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn;
> +    Error *local_err = NULL;
>      char *filename, *snapshot_name = NULL;
>      int c, ret = 0, bdrv_oflags;
>      int action = 0;
> @@ -2033,11 +2034,9 @@ static int img_snapshot(int argc, char **argv)
>          break;
>  
>      case SNAPSHOT_DELETE:
> -        ret = bdrv_snapshot_delete(bs, snapshot_name);
> -        if (ret) {
> -            error_report("Could not delete snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> -        }
> +        bdrv_snapshot_delete(bs, snapshot_name, &local_err);
> +        ret = qemu_img_handle_error("qemu-img snapshot delete failed",
> +                                    local_err);

Here again "qemu-img snapshot delete failed" is redundant. If this is
the command that I typed and I get an error, then it's obvious that it's
this command that failed.

>          break;
>      }
>  
> 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:

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().

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

Kevin
Eric Blake - April 18, 2013, 1:09 p.m.
On 04/18/2013 06:55 AM, Kevin Wolf wrote:
> Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>  
>> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>> +void bdrv_snapshot_delete(BlockDriverState *bs,
>> +                          const char *snapshot_id,
>> +                          Error **errp)
>>  {
>>      BlockDriver *drv = bs->drv;
>> -    if (!drv)
>> -        return -ENOMEDIUM;
>> -    if (drv->bdrv_snapshot_delete)
>> -        return drv->bdrv_snapshot_delete(bs, snapshot_id);
>> -    if (bs->file)
>> -        return bdrv_snapshot_delete(bs->file, snapshot_id);
>> -    return -ENOTSUP;
>> +
>> +    if (!drv) {
>> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
> 
> I don't think the device name is useful here. It's always the device
> that the user has specified in the monitor command.

Huh?  We're talking about vm-snapshot-delete, which removes the snapshot
for multiple devices at a time.  Knowing _which_ device failed is
important in the context of a command where you don't specify a device name.

> 
> Also, please start error messages with a capital letter. (This applies
> to the whole patch, and probably also other patches in this series)

Qemu is inconsistent on this front.  The code base actually favors lower
case at the moment:

$ git grep 'error_setg.*"[a-z]' | wc
    119     957   10361
$ git grep 'error_setg.*"[A-Z]' | wc
     60     510    4996

Monitor output, on the other hand, favor uppercase:

$ git grep 'monitor_pr.*"[A-Z]' | wc
     88     576    6611
$ git grep 'monitor_pr.*"[a-z]' | wc
    108     789    8566

If you want to enforce a particular style, I think it would be best to
patch HACKING to document a preferred style.

If it helps as a tie breaker, GNU Coding Standards recommend lowercase:
https://www.gnu.org/prep/standards/standards.html#Errors

Personally, I don't care which way we go.

> 
>> +    } else if (drv->bdrv_snapshot_delete) {
>> +        drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
>> +    } else if (bs->file) {
>> +        bdrv_snapshot_delete(bs->file, snapshot_id, errp);
>> +    } else {
>> +        error_setg(errp, "snapshots are not supported on device '%s'",
>> +                   bdrv_get_device_name(bs));
> 
> Same thing with the device name here.

Same thing about this function being reached via vm-snapshot-delete
where we aren't passing in a device name, so knowing which device failed
is important.
Pavel Hrdina - April 18, 2013, 1:19 p.m.
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
>
Kevin Wolf - April 18, 2013, 1:41 p.m.
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
Kevin Wolf - April 18, 2013, 1:51 p.m.
Am 18.04.2013 um 15:09 hat Eric Blake geschrieben:
> On 04/18/2013 06:55 AM, Kevin Wolf wrote:
> > Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> >>
> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >> ---
> >>  
> >> -int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> >> +void bdrv_snapshot_delete(BlockDriverState *bs,
> >> +                          const char *snapshot_id,
> >> +                          Error **errp)
> >>  {
> >>      BlockDriver *drv = bs->drv;
> >> -    if (!drv)
> >> -        return -ENOMEDIUM;
> >> -    if (drv->bdrv_snapshot_delete)
> >> -        return drv->bdrv_snapshot_delete(bs, snapshot_id);
> >> -    if (bs->file)
> >> -        return bdrv_snapshot_delete(bs->file, snapshot_id);
> >> -    return -ENOTSUP;
> >> +
> >> +    if (!drv) {
> >> +        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
> > 
> > I don't think the device name is useful here. It's always the device
> > that the user has specified in the monitor command.
> 
> Huh?  We're talking about vm-snapshot-delete, which removes the snapshot
> for multiple devices at a time.  Knowing _which_ device failed is
> important in the context of a command where you don't specify a device name.

No, we're talking about bdrv_snapshot_delete(). Talking about
vm-snapshot-delete in this patch would be a layering violation.

If it's important for vm-snapshot-delete to have the device name in the
error message (and I agree it is when deleting multiple snapshots), then
qmp_vm_snapshot_delete() should add that information.

> > 
> > Also, please start error messages with a capital letter. (This applies
> > to the whole patch, and probably also other patches in this series)
> 
> Qemu is inconsistent on this front.  The code base actually favors lower
> case at the moment:
> 
> $ git grep 'error_setg.*"[a-z]' | wc
>     119     957   10361
> $ git grep 'error_setg.*"[A-Z]' | wc
>      60     510    4996
> 
> Monitor output, on the other hand, favor uppercase:
> 
> $ git grep 'monitor_pr.*"[A-Z]' | wc
>      88     576    6611
> $ git grep 'monitor_pr.*"[a-z]' | wc
>     108     789    8566

If you don't pipe it through wc but look at the output, you'll see that
many of these start with a lower case device, module or parameter
name, which of course wouldn't be right with upper case.

So far I think the block layer messages are pretty consistently upper
case where it makes sense.

Kevin

Patch

diff --git a/block.c b/block.c
index 4ad663d..fb065e6 100644
--- a/block.c
+++ b/block.c
@@ -3391,16 +3391,22 @@  int bdrv_snapshot_goto(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+void bdrv_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_delete)
-        return drv->bdrv_snapshot_delete(bs, snapshot_id);
-    if (bs->file)
-        return bdrv_snapshot_delete(bs->file, snapshot_id);
-    return -ENOTSUP;
+
+    if (!drv) {
+        error_setg(errp, "device '%s' has no medium", bdrv_get_device_name(bs));
+    } else if (drv->bdrv_snapshot_delete) {
+        drv->bdrv_snapshot_delete(bs, snapshot_id, errp);
+    } else if (bs->file) {
+        bdrv_snapshot_delete(bs->file, snapshot_id, errp);
+    } else {
+        error_setg(errp, "snapshots are not supported on device '%s'",
+                   bdrv_get_device_name(bs));
+    }
 }
 
 int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 992a5c8..2ebeadc 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -530,7 +530,9 @@  fail:
     return ret;
 }
 
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+void qcow2_snapshot_delete(BlockDriverState *bs,
+                           const char *snapshot_id,
+                           Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot sn;
@@ -539,7 +541,9 @@  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     /* Search the snapshot */
     snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
     if (snapshot_index < 0) {
-        return -ENOENT;
+        error_setg(errp, "qcow2: failed to find snapshot '%s' by id or name",
+                   snapshot_id);
+        return;
     }
     sn = s->snapshots[snapshot_index];
 
@@ -550,7 +554,9 @@  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     s->nb_snapshots--;
     ret = qcow2_write_snapshots(bs);
     if (ret < 0) {
-        return ret;
+        error_setg_errno(errp, -ret, "qcow2: failed to remove snapshot '%s' "
+                         "from snapshot list", sn.name);
+        return;
     }
 
     /*
@@ -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");
+        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");
+        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));
+    }
 }
 
 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;
 }
 
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static void sd_snapshot_delete(BlockDriverState *bs,
+                               const char *snapshot_id,
+                               Error **errp)
 {
     /* FIXME: Delete specified snapshot id.  */
-    return 0;
+    return;
 }
 
 static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
diff --git a/include/block/block.h b/include/block/block.h
index ebd9512..db8c6aa 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -337,7 +337,9 @@  int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+void bdrv_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 458cde3..53c52bb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -157,7 +157,9 @@  struct BlockDriver {
                                 QEMUSnapshotInfo *sn_info);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
-    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
+    void (*bdrv_snapshot_delete)(BlockDriverState *bs,
+                                 const char *snapshot_id,
+                                 Error **errp);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index dbacdb7..4828fe4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1936,6 +1936,7 @@  static int img_snapshot(int argc, char **argv)
 {
     BlockDriverState *bs;
     QEMUSnapshotInfo sn;
+    Error *local_err = NULL;
     char *filename, *snapshot_name = NULL;
     int c, ret = 0, bdrv_oflags;
     int action = 0;
@@ -2033,11 +2034,9 @@  static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_DELETE:
-        ret = bdrv_snapshot_delete(bs, snapshot_name);
-        if (ret) {
-            error_report("Could not delete snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
-        }
+        bdrv_snapshot_delete(bs, snapshot_name, &local_err);
+        ret = qemu_img_handle_error("qemu-img snapshot delete failed",
+                                    local_err);
         break;
     }
 
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));
+                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));
+                error_free(local_err);
             }
         }
     }