Re: [Qemu-devel] [PATCH 25/25] qcow2-bitmap: improve check_constraints_on_bitmap

[John Snow]

On 02/14/2017 11:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add detailed error messages.
> 

yay

> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2-bitmap.c | 63 
> ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 7ad1f7c..113d48c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -160,28 +160,49 @@ static int check_table_entry(uint64_t entry, int 
> cluster_size)
>  
>  static int check_constraints_on_bitmap(BlockDriverState *bs,
>                                         const char *name,
> -                                       uint32_t granularity)
> +                                       uint32_t granularity,
> +                                       Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      int granularity_bits = ctz32(granularity);
>      int64_t len = bdrv_getlength(bs);
> -    bool fail;
>  
>      assert(granularity > 0);
>      assert((granularity & (granularity - 1)) == 0);
>  
>      if (len < 0) {
> +        error_setg_errno(errp, -len, "Failed to get size of '%s'",
> +                         bdrv_get_device_or_node_name(bs));
>          return len;
>      }
>  
> -    fail = (granularity_bits > BME_MAX_GRANULARITY_BITS) ||
> -           (granularity_bits < BME_MIN_GRANULARITY_BITS) ||
> -           (len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> -           (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> -                  granularity_bits) ||
> -           (strlen(name) > BME_MAX_NAME_SIZE);
> +    if (granularity_bits > BME_MAX_GRANULARITY_BITS) {
> +        error_setg(errp, "Granularity exceeds maximum (%u bytes)",
> +                   1 << BME_MAX_GRANULARITY_BITS);
> +        return -EINVAL;
> +    }
> +    if (granularity_bits < BME_MIN_GRANULARITY_BITS) {
> +        error_setg(errp, "Granularity is under minimum (%u bytes)",
> +                   1 << BME_MIN_GRANULARITY_BITS);
> +        return -EINVAL;
> +    }
>  
> -    return fail ? -EINVAL : 0;
> +    if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
> +        (len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
> +               granularity_bits))
> +    {
> +        error_setg(errp, "Too much space will be occupied by the bitmap. "
> +                   "Use larger granularity");
> +        return -EINVAL;
> +    }
> +
> +    if (strlen(name) > BME_MAX_NAME_SIZE) {
> +        error_setg(errp, "Name length exceeds maximum (%u characters)",
> +                   BME_MAX_NAME_SIZE);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
>  }
>  
>  static void clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
> @@ -1142,9 +1163,9 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>              continue;
>          }
>  
> -        if (check_constraints_on_bitmap(bs, name, granularity) < 0) {
> -            error_setg(errp, "Bitmap '%s' doesn't satisfy the constraints",
> -                       name);
> +        if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
> +            error_prepend(errp, "Bitmap '%s' doesn't satisfy the 
> constraints: ",
> +                          name);
>              goto fail;
>          }
>  
> @@ -1230,13 +1251,11 @@ bool 
> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>                                        Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    const char *reason = NULL;
>      bool found;
>      Qcow2BitmapList *bm_list;
>  
> -    if (check_constraints_on_bitmap(bs, name, granularity) != 0) {
> -        reason = "it doesn't satisfy the constraints";
> -        goto common_errp;
> +    if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
> +        goto fail;
>      }
>  
>      if (s->nb_bitmaps == 0) {
> @@ -1246,21 +1265,21 @@ bool 
> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>      bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
>                                 s->bitmap_directory_size, errp);
>      if (bm_list == NULL) {
> -        return false;
> +        goto fail;
>      }
>  
>      found = find_bitmap_by_name(bm_list, name);
>      bitmap_list_free(bm_list);
>      if (found) {
> -        reason = "bitmap with the same name is already stored";
> -        goto common_errp;
> +        error_setg(errp, "Bitmap with the same name is already stored");
> +        goto fail;
>      }
>  
>      return true;
>  
> -common_errp:
> -    error_setg(errp, "Can't make bitmap '%s' persistent in '%s', as %s.",
> -               name, bdrv_get_device_or_node_name(bs), reason);
> +fail:
> +    error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
> +                  name, bdrv_get_device_or_node_name(bs));
>      return false;
>  }
>  
> 

Sorry I made it less efficient :)

Reviewed-by: John Snow <address@hidden>