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

[John Snow]

On 02/15/2017 05:10 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add detailed error messages.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>

Reviewed-by: John Snow <address@hidden>

> ---
>  block/qcow2-bitmap.c | 48 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 9177c56..e25c872 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;
>          }
>  
> @@ -1233,8 +1254,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState 
> *bs,
>      bool found;
>      Qcow2BitmapList *bm_list;
>  
> -    if (check_constraints_on_bitmap(bs, name, granularity) != 0) {
> -        error_setg(errp, "The constraints are not satisfied");
> +    if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
>          goto fail;
>      }
>  
> 

-- 
—js