Re: [Qemu-devel] [PATCH] block: Don't throw away errno via error_setg

[Fam Zheng]

On Wed, 02/12 14:46, Jeff Cody wrote:
> There are a handful of places in the block layer where a failure path
> has a valid -errno value, yet error_setg() is used.  Those instances
> should instead use error_setg_errno(), to preserve as much error
> information as possible.
> 
> This patch replaces those instances with error_setg_errno(), so that
> errno is passed up the stack in the error message.
> 
> Reported-By: Kevin Wolf <address@hidden>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/mirror.c         | 13 +++++++++----
>  block/qcow2-snapshot.c |  8 +++++---
>  block/vmdk.c           |  6 +++---
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 41bb83c..b31d840 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -633,6 +633,7 @@ void commit_active_start(BlockDriverState *bs, 
> BlockDriverState *base,
>  {
>      int64_t length, base_length;
>      int orig_base_flags;
> +    int ret;
>  
>      assert(errp != NULL);
>  
> @@ -644,19 +645,23 @@ void commit_active_start(BlockDriverState *bs, 
> BlockDriverState *base,
>  
>      length = bdrv_getlength(bs);
>      if (length < 0) {
> -        error_setg(errp, "Unable to determine length of %s", bs->filename);
> +        error_setg_errno(errp, -length,
> +                         "Unable to determine length of %s", bs->filename);
>          goto error_restore_flags;
>      }
>  
>      base_length = bdrv_getlength(base);
>      if (base_length < 0) {
> -        error_setg(errp, "Unable to determine length of %s", base->filename);
> +        error_setg_errno(errp, -base_length,
> +                         "Unable to determine length of %s", base->filename);
>          goto error_restore_flags;
>      }
>  
>      if (length > base_length) {
> -        if (bdrv_truncate(base, length) < 0) {
> -            error_setg(errp, "Top image %s is larger than base image %s, and 
> "
> +        ret = bdrv_truncate(base, length);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret,
> +                            "Top image %s is larger than base image %s, and "
>                               "resize of base image failed",
>                               bs->filename, base->filename);
>              goto error_restore_flags;
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index ad8bf3d..2fc6320 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -606,7 +606,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
>      s->nb_snapshots--;
>      ret = qcow2_write_snapshots(bs);
>      if (ret < 0) {
> -        error_setg(errp, "Failed to remove snapshot from snapshot list");
> +        error_setg_errno(errp, -ret,
> +                         "Failed to remove snapshot from snapshot list");
>          return ret;
>      }
>  
> @@ -624,7 +625,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
>      ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
>                                           sn.l1_size, -1);
>      if (ret < 0) {
> -        error_setg(errp, "Failed to free the cluster and L1 table");
> +        error_setg_errno(errp, -ret, "Failed to free the cluster and L1 
> table");
>          return ret;
>      }
>      qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * 
> sizeof(uint64_t),
> @@ -633,7 +634,8 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
>      /* 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) {
> -        error_setg(errp, "Failed to update snapshot status in disk");
> +        error_setg_errno(errp, -ret,
> +                         "Failed to update snapshot status in disk");
>          return ret;
>      }
>  
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e809e2e..ff6f5ee 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1502,7 +1502,7 @@ static int vmdk_create_extent(const char *filename, 
> int64_t filesize,
>      if (flat) {
>          ret = bdrv_truncate(bs, filesize);
>          if (ret < 0) {
> -            error_setg(errp, "Could not truncate file");
> +            error_setg_errno(errp, -ret, "Could not truncate file");
>          }
>          goto exit;
>      }
> @@ -1562,7 +1562,7 @@ static int vmdk_create_extent(const char *filename, 
> int64_t filesize,
>  
>      ret = bdrv_truncate(bs, le64_to_cpu(header.grain_offset) << 9);
>      if (ret < 0) {
> -        error_setg(errp, "Could not truncate file");
> +        error_setg_errno(errp, -ret, "Could not truncate file");
>          goto exit;
>      }
>  
> @@ -1846,7 +1846,7 @@ static int vmdk_create(const char *filename, 
> QEMUOptionParameter *options,
>      if (desc_offset == 0) {
>          ret = bdrv_truncate(new_bs, desc_len);
>          if (ret < 0) {
> -            error_setg(errp, "Could not truncate file");
> +            error_setg_errno(errp, -ret, "Could not truncate file");
>          }
>      }
>  exit:
> -- 
> 1.8.3.1
> 

Reviewed-by: Fam Zheng <address@hidden>