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

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

[Jeff Cody]

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 <kwolf@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On 02/12/2014 12:46 PM, 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 <kwolf@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/mirror.c         | 13 +++++++++----
>  block/qcow2-snapshot.c |  8 +++++---
>  block/vmdk.c           |  6 +++---
>  3 files changed, 17 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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 <kwolf@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  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 <famz@redhat.com>

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

[Kevin Wolf]

Am 12.02.2014 um 20:46 hat Jeff Cody geschrieben:
> 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 <kwolf@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Stefan Hajnoczi]

On Wed, Feb 12, 2014 at 02:46:24PM -0500, 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 <kwolf@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/mirror.c         | 13 +++++++++----
>  block/qcow2-snapshot.c |  8 +++++---
>  block/vmdk.c           |  6 +++---
>  3 files changed, 17 insertions(+), 10 deletions(-)

You are a lucky man, getting so many Reviewed-by: lines.  I think this
is a record!

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan