[Qemu-trivial] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Qemu-trivial] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Chen Gang]

When failure occurs, 'ret' need be set, or may return 0 to indicate success.
And error_propagate() also need be called only one time within a function.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 74af8d7..12ce4e4 100644
--- a/block.c
+++ b/block.c
@@ -1497,7 +1497,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     if (snapshot_flags) {
         bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
+            ret = -1;
             goto close_and_fail;
         }
     }
-- 
1.7.11.7

[Qemu-devel] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Chen Gang]

When failure occurs, 'ret' need be set, or may return 0 to indicate success.
And error_propagate() also need be called only one time within a function.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 74af8d7..12ce4e4 100644
--- a/block.c
+++ b/block.c
@@ -1497,7 +1497,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     if (snapshot_flags) {
         bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
+            ret = -1;
             goto close_and_fail;
         }
     }
-- 
1.7.11.7

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Stefan Hajnoczi]

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

On Sat, Jun 21, 2014 at 10:59:29PM +0800, Chen Gang wrote:
> When failure occurs, 'ret' need be set, or may return 0 to indicate success.
> And error_propagate() also need be called only one time within a function.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 74af8d7..12ce4e4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1497,7 +1497,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      if (snapshot_flags) {
>          bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
> +            ret = -1;
>              goto close_and_fail;
>          }
>      }

The return value of bdrv_open() is a negative errno.  -1 does not make
sense in this case.

Please add an int return value to bdrv_append_temp_snapshot() so the
caller can determine the real errno.  Then the remainder of the patch
can be:

     if (snapshot_flags) {
-        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
+        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
             goto close_and_fail;
         }
     }

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Stefan Hajnoczi]

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

On Sat, Jun 21, 2014 at 10:59:29PM +0800, Chen Gang wrote:
> When failure occurs, 'ret' need be set, or may return 0 to indicate success.
> And error_propagate() also need be called only one time within a function.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 74af8d7..12ce4e4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1497,7 +1497,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      if (snapshot_flags) {
>          bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
> +            ret = -1;
>              goto close_and_fail;
>          }
>      }

The return value of bdrv_open() is a negative errno.  -1 does not make
sense in this case.

Please add an int return value to bdrv_append_temp_snapshot() so the
caller can determine the real errno.  Then the remainder of the patch
can be:

     if (snapshot_flags) {
-        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
+        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
-            error_propagate(errp, local_err);
             goto close_and_fail;
         }
     }

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-trivial] [Qemu-devel] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Chen Gang]

On 06/23/2014 03:38 PM, Stefan Hajnoczi wrote:
> 
> The return value of bdrv_open() is a negative errno.  -1 does not make
> sense in this case.
> 
> Please add an int return value to bdrv_append_temp_snapshot() so the
> caller can determine the real errno.  Then the remainder of the patch
> can be:
> 
>      if (snapshot_flags) {
> -        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
> +        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
>              goto close_and_fail;
>          }
>      }
> 

That's fine to me, I shall send patch v2 for it.

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed

Re: [Qemu-devel] [PATCH trivial] block.c: Correct the return value and the failure object when failure occurs

[Chen Gang]

On 06/23/2014 03:38 PM, Stefan Hajnoczi wrote:
> 
> The return value of bdrv_open() is a negative errno.  -1 does not make
> sense in this case.
> 
> Please add an int return value to bdrv_append_temp_snapshot() so the
> caller can determine the real errno.  Then the remainder of the patch
> can be:
> 
>      if (snapshot_flags) {
> -        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
> +        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
>          if (local_err) {
> -            error_propagate(errp, local_err);
>              goto close_and_fail;
>          }
>      }
> 

That's fine to me, I shall send patch v2 for it.

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed