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 <address@hidden>
> ---
>  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 <address@hidden>

Fam

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