[Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

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

It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
set errp when error occurs -- although it contents return value internally.

So let bdrv_append_temp_snapshot() internal return value outside, and let
all things normal, then fix the issue too.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 block.c               | 7 ++++---
 include/block/block.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 74af8d7..5e4fb60 100644
--- a/block.c
+++ b/block.c
@@ -1240,7 +1240,7 @@ done:
     return ret;
 }
 
-void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
+int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 {
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);
@@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
     /* Get the required size from the image */
     total_size = bdrv_getlength(bs);
     if (total_size < 0) {
+        ret = total_size;
         error_setg_errno(errp, -total_size, "Could not get image size");
         goto out;
     }
@@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
 
 out:
     g_free(tmp_filename);
+    return ret;
 }
 
 static QDict *parse_json_filename(const char *filename, Error **errp)
@@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
     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;
         }
     }
diff --git a/include/block/block.h b/include/block/block.h
index f15b99b..7b3381c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
                     bool allow_none, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
-void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
+int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
 int bdrv_open(BlockDriverState **pbs, const char *filename,
               const char *reference, QDict *options, int flags,
               BlockDriver *drv, Error **errp);
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Fam Zheng]

On Mon, 06/23 23:28, Chen Gang wrote:
> When failure occurs, 'ret' need be set, or may return 0 to indicate success.

s/need/needs/

> And error_propagate() also need be called only one time within a function.

s/need/needs/

> 
> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
> set errp when error occurs -- although it contents return value internally.

s/contents/has/

> 
> So let bdrv_append_temp_snapshot() internal return value outside, and let
> all things normal, then fix the issue too.
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  block.c               | 7 ++++---
>  include/block/block.h | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 74af8d7..5e4fb60 100644
> --- a/block.c
> +++ b/block.c
> @@ -1240,7 +1240,7 @@ done:
>      return ret;
>  }
>  
> -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
> +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>  {
>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
> @@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>      /* Get the required size from the image */
>      total_size = bdrv_getlength(bs);
>      if (total_size < 0) {
> +        ret = total_size;
>          error_setg_errno(errp, -total_size, "Could not get image size");
>          goto out;
>      }
> @@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>  
>  out:
>      g_free(tmp_filename);
> +    return ret;
>  }
>  
>  static QDict *parse_json_filename(const char *filename, Error **errp)
> @@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>      /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
>       * temporary snapshot afterwards. */
>      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;
>          }
>      }
> diff --git a/include/block/block.h b/include/block/block.h
> index f15b99b..7b3381c 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>                      bool allow_none, Error **errp);
>  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
> -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
> +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);

Not used outside block.c, but pre-existing.

Reviewed-by: Fam Zheng <famz@redhat.com>

Fam

>  int bdrv_open(BlockDriverState **pbs, const char *filename,
>                const char *reference, QDict *options, int flags,
>                BlockDriver *drv, Error **errp);
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Chen Gang]

On 06/24/2014 10:25 AM, Fam Zheng wrote:
> On Mon, 06/23 23:28, Chen Gang wrote:
>> When failure occurs, 'ret' need be set, or may return 0 to indicate success.
> 
> s/need/needs/
> 
>> And error_propagate() also need be called only one time within a function.
> 
> s/need/needs/
> 

For a grammar in my learning, in this situation, 'need' is not a verb,
"be set" and "be called" are the verb used with passive mode.

>>
>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
>> set errp when error occurs -- although it contents return value internally.
> 
> s/contents/has/
> 

OK, thanks, and excuse me for my poor English. If necessary I shall send
patch v2 for it.

>>
>> So let bdrv_append_temp_snapshot() internal return value outside, and let
>> all things normal, then fix the issue too.
>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  block.c               | 7 ++++---
>>  include/block/block.h | 2 +-
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 74af8d7..5e4fb60 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1240,7 +1240,7 @@ done:
>>      return ret;
>>  }
>>  
>> -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>> +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>>  {
>>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
>> @@ -1258,6 +1258,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>>      /* Get the required size from the image */
>>      total_size = bdrv_getlength(bs);
>>      if (total_size < 0) {
>> +        ret = total_size;
>>          error_setg_errno(errp, -total_size, "Could not get image size");
>>          goto out;
>>      }
>> @@ -1304,6 +1305,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>>  
>>  out:
>>      g_free(tmp_filename);
>> +    return ret;
>>  }
>>  
>>  static QDict *parse_json_filename(const char *filename, Error **errp)
>> @@ -1495,9 +1497,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
>>      /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
>>       * temporary snapshot afterwards. */
>>      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;
>>          }
>>      }
>> diff --git a/include/block/block.h b/include/block/block.h
>> index f15b99b..7b3381c 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -217,7 +217,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename,
>>                      bool allow_none, Error **errp);
>>  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
>>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
>> -void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
>> +int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
> 
> Not used outside block.c, but pre-existing.
> 

Yeah, originally, I considered about it, and finally still remained it
as pre-existing.

> Reviewed-by: Fam Zheng <famz@redhat.com>
> 
Thank you for your work.

Thanks.
-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Kevin Wolf]

Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
> 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.
> 
> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
> set errp when error occurs -- although it contents return value internally.
> 
> So let bdrv_append_temp_snapshot() internal return value outside, and let
> all things normal, then fix the issue too.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

What does this fix?

Having both a return value and an Error* object is duplication and
only a sign that a function hasn't been fully converted to the Error
framework yet. We shouldn't introduce new instances of this without a
very good reason.

Kevin

Re: [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
>> 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.
>> 
>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
>> set errp when error occurs -- although it contents return value internally.
>> 
>> So let bdrv_append_temp_snapshot() internal return value outside, and let
>> all things normal, then fix the issue too.
>> 
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>
> What does this fix?

It fixes the return value of bdrv_open() when
bdrv_append_temp_snapshot() fails.  Before this patch, it returns a
positive value, which is wrong.  After the patch, it returns the
negative error code bdrv_append_temp_snapshot() now returns.

> Having both a return value and an Error* object is duplication and
> only a sign that a function hasn't been fully converted to the Error
> framework yet. We shouldn't introduce new instances of this without a
> very good reason.

Maybe.  But I very much prefer

        ret = foo(arg, errp);
        if (ret < 0) {
            return ret;
        }

over

        Error *local_err = NULL;

        foo(arg, &local_err);
        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }

Re: [Qemu-devel] [Qemu-trivial] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Michael Tokarev]

24.06.2014 15:01, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
>>> 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.
>>>
>>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
>>> set errp when error occurs -- although it contents return value internally.
>>>
>>> So let bdrv_append_temp_snapshot() internal return value outside, and let
>>> all things normal, then fix the issue too.
>>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> What does this fix?
> 
> It fixes the return value of bdrv_open() when
> bdrv_append_temp_snapshot() fails.  Before this patch, it returns a
> positive value, which is wrong.  After the patch, it returns the
> negative error code bdrv_append_temp_snapshot() now returns.

So, what should be done there?   Kevin, maybe you should pick this up
instead of going -trivial route?

>> Having both a return value and an Error* object is duplication and
>> only a sign that a function hasn't been fully converted to the Error
>> framework yet. We shouldn't introduce new instances of this without a
>> very good reason.
> 
> Maybe.  But I very much prefer
> 
>         ret = foo(arg, errp);
>         if (ret < 0) {
>             return ret;
>         }
> 
> over
> 
>         Error *local_err = NULL;
> 
>         foo(arg, &local_err);
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
>         }

Yes, this new error propagation is a bit ugly, I dislike it too.

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Chen Gang]

Thank you for all of your work, if necessary to send patch v3 for it
(may change the comments), please let me know.

Thanks.

On 06/25/2014 12:00 AM, Michael Tokarev wrote:
> 24.06.2014 15:01, Markus Armbruster wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
>>>> 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.
>>>>
>>>> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
>>>> set errp when error occurs -- although it contents return value internally.
>>>>
>>>> So let bdrv_append_temp_snapshot() internal return value outside, and let
>>>> all things normal, then fix the issue too.
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>
>>> What does this fix?
>>
>> It fixes the return value of bdrv_open() when
>> bdrv_append_temp_snapshot() fails.  Before this patch, it returns a
>> positive value, which is wrong.  After the patch, it returns the
>> negative error code bdrv_append_temp_snapshot() now returns.
> 
> So, what should be done there?   Kevin, maybe you should pick this up
> instead of going -trivial route?
> 
>>> Having both a return value and an Error* object is duplication and
>>> only a sign that a function hasn't been fully converted to the Error
>>> framework yet. We shouldn't introduce new instances of this without a
>>> very good reason.
>>
>> Maybe.  But I very much prefer
>>
>>         ret = foo(arg, errp);
>>         if (ret < 0) {
>>             return ret;
>>         }
>>
>> over
>>
>>         Error *local_err = NULL;
>>
>>         foo(arg, &local_err);
>>         if (local_err) {
>>             error_propagate(errp, local_err);
>>             return;
>>         }
> 
> Yes, this new error propagation is a bit ugly, I dislike it too.
> 
> Thanks,
> 
> /mjt
> 


-- 
Chen Gang

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

Re: [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Stefan Hajnoczi]

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

On Tue, Jun 24, 2014 at 01:01:52PM +0200, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
> >> 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.
> >> 
> >> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
> >> set errp when error occurs -- although it contents return value internally.
> >> 
> >> So let bdrv_append_temp_snapshot() internal return value outside, and let
> >> all things normal, then fix the issue too.
> >> 
> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> >
> > What does this fix?
> 
> It fixes the return value of bdrv_open() when
> bdrv_append_temp_snapshot() fails.  Before this patch, it returns a
> positive value, which is wrong.  After the patch, it returns the
> negative error code bdrv_append_temp_snapshot() now returns.

Exactly.  I asked for the -errno return because otherwise bdrv_open()
would have no accurate errno.

Stefan

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

Re: [Qemu-devel] [PATCH trivial v2] block.c: Add return value for bdrv_append_temp_snapshot() to avoid incorrect failure processing issue

[Kevin Wolf]

Am 23.06.2014 um 17:28 hat Chen Gang geschrieben:
> 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.
> 
> It is abnormal to prevent bdrv_append_temp_snapshot() return value but still
> set errp when error occurs -- although it contents return value internally.
> 
> So let bdrv_append_temp_snapshot() internal return value outside, and let
> all things normal, then fix the issue too.
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Thanks, applied to the block branch.

Kevin