Re: [PATCH 12/14] block/qcow2: read_cache_sizes: return status value

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:28 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> It's better to return status together with setting errp. It allows to
> reduce error propagation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  block/qcow2.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c2cd9434cc..31dd28d19e 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -869,7 +869,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs,
>      cache_clean_timer_init(bs, new_context);
>  }
>  
> -static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
> +static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
>                               uint64_t *l2_cache_size,
>                               uint64_t *l2_cache_entry_size,
>                               uint64_t *refcount_cache_size, Error **errp)
> @@ -907,16 +907,16 @@ static void read_cache_sizes(BlockDriverState *bs, 
> QemuOpts *opts,
>              error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " 
> QCOW2_OPT_L2_CACHE_SIZE
>                         " and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be 
> set "
>                         "at the same time");
> -            return;
> +            return false;
>          } else if (l2_cache_size_set &&
>                     (l2_cache_max_setting > combined_cache_size)) {
>              error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
>                         QCOW2_OPT_CACHE_SIZE);
> -            return;
> +            return false;
>          } else if (*refcount_cache_size > combined_cache_size) {
>              error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed "
>                         QCOW2_OPT_CACHE_SIZE);
> -            return;
> +            return false;
>          }
>  
>          if (l2_cache_size_set) {
> @@ -955,8 +955,10 @@ static void read_cache_sizes(BlockDriverState *bs, 
> QemuOpts *opts,
>          error_setg(errp, "L2 cache entry size must be a power of two "
>                     "between %d and the cluster size (%d)",
>                     1 << MIN_CLUSTER_BITS, s->cluster_size);
> -        return;
> +        return false;
>      }
> +
> +    return true;
>  }
>  
>  typedef struct Qcow2ReopenState {
> @@ -983,7 +985,6 @@ static int qcow2_update_options_prepare(BlockDriverState 
> *bs,
>      int i;
>      const char *encryptfmt;
>      QDict *encryptopts = NULL;
> -    Error *local_err = NULL;
>      int ret;
>  
>      qdict_extract_subqdict(options, &encryptopts, "encrypt.");
> @@ -996,10 +997,8 @@ static int qcow2_update_options_prepare(BlockDriverState 
> *bs,
>      }
>  
>      /* get L2 table/refcount block cache size from command line options */
> -    read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
> -                     &refcount_cache_size, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
> +                          &refcount_cache_size, errp)) {
>          ret = -EINVAL;
>          goto fail;
>      }