Re: [Qemu-block] [PATCH v11 1/3] bdrv_query_image_info Error parameter added

[Vladimir Sementsov-Ogievskiy]

31.01.2019 16:46, Andrey Shinkevich wrote:
> Inform a user in case qcow2_get_specific_info fails to obtain
> QCOW2 image specific information. This patch is preliminary to
> the print of bitmap information in the 'qemu-img info' output.
> 
> Signed-off-by: Andrey Shinkevich <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---


[...]

> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -594,13 +594,13 @@ static int block_crypto_get_info_luks(BlockDriverState 
> *bs,
>   }
>   
>   static ImageInfoSpecific *
> -block_crypto_get_specific_info_luks(BlockDriverState *bs)
> +block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
>   {
>       BlockCrypto *crypto = bs->opaque;
>       ImageInfoSpecific *spec_info;
>       QCryptoBlockInfo *info;
>   
> -    info = qcrypto_block_get_info(crypto->block, NULL);
> +    info = qcrypto_block_get_info(crypto->block, errp);
>       if (!info) {
>           return NULL;
>       }

more context:

       if (info->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
           qapi_free_QCryptoBlockInfo(info);
           return NULL;
       }

for a fast look, I think it should be assertion, not if, Daniel, am I right?
Also, I think we don't have block/crypto.c in Cryptography section of 
MAINTAINERS
by mistake, so you were not CC'ed.


> diff --git a/block/qapi.c b/block/qapi.c
> index c66f949..f53f100 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -282,7 +282,12 @@ void bdrv_query_image_info(BlockDriverState *bs,
>           info->dirty_flag = bdi.is_dirty;
>           info->has_dirty_flag = true;
>       }
> -    info->format_specific     = bdrv_get_specific_info(bs);
> +    info->format_specific     = bdrv_get_specific_info(bs, &err);

while being here, let's drop these extra whitespaces

> +    if (err) {
> +        error_propagate(errp, err);
> +        qapi_free_ImageInfo(info);
> +        goto out;
> +    }
>       info->has_format_specific = info->format_specific != NULL;
>   
>       backing_filename = bs->backing_file;


So, I'm unsure about crypto, but it's safe to keep it as is, so for this patch:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>



-- 
Best regards,
Vladimir