diff mbox series

[v11,1/3] bdrv_query_image_info Error parameter added

Message ID 1548942405-760115-2-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series qemu-img info lists bitmap directory entries | expand

Commit Message

Andrey Shinkevich Jan. 31, 2019, 1:46 p.m. UTC 
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 <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   |  5 +++--
 block/crypto.c            |  4 ++--
 block/qapi.c              |  7 ++++++-
 block/qcow2.c             | 10 ++++++++--
 block/vmdk.c              |  3 ++-
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qemu-io-cmds.c            |  7 ++++++-
 8 files changed, 31 insertions(+), 11 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Feb. 1, 2019, 2:42 p.m. UTC | #1 
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 <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---


[...]

> --- 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 <vsementsov@virtuozzo.com>
Daniel P. Berrangé Feb. 1, 2019, 2:46 p.m. UTC | #2 
On Fri, Feb 01, 2019 at 02:42:10PM +0000, Vladimir Sementsov-Ogievskiy wrote:
> 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 <andrey.shinkevich@virtuozzo.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> 
> 
> [...]
> 
> > --- 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?

Sure, it could be an assertion. In practice it should never trigger
eitherway.

> Also, I think we don't have block/crypto.c in Cryptography section of MAINTAINERS
> by mistake, so you were not CC'ed.

I tend to let the block maintainers merge patches under block/,
since that's largely block layer integration. The guts of the
block crypto stuff is under crypto/


Regards,
Daniel
Kevin Wolf Feb. 1, 2019, 5:13 p.m. UTC | #3 
Am 31.01.2019 um 14:46 hat Andrey Shinkevich geschrieben:
> 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 <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/block/crypto.c b/block/crypto.c
> index f0a5f6b..4ede833 100644
> --- 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;
>      }

In v9 of the series I suggested to have another patch to remove errp
from qcrypto_block_get_info() because it never returns errors anyway.

Don't you think that would be better? But it's your decision, I'm fine
with either way.

> 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);

The spacing looks odd now. I'd either keep the assignment for
info->has_format_specific above the if block so that both are aligned to
the same column, or remove the additional spaces.

With that fixed:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff mbox series

Patch

diff --git a/block.c b/block.c
index 4f5ff2c..1eb35ef 100644
--- a/block.c
+++ b/block.c
@@ -4307,11 +4307,12 @@  int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return drv->bdrv_get_info(bs, bdi);
 }
 
-ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs)
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
+                                          Error **errp)
 {
     BlockDriver *drv = bs->drv;
     if (drv && drv->bdrv_get_specific_info) {
-        return drv->bdrv_get_specific_info(bs);
+        return drv->bdrv_get_specific_info(bs, errp);
     }
     return NULL;
 }
diff --git a/block/crypto.c b/block/crypto.c
index f0a5f6b..4ede833 100644
--- 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;
     }
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);
+    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;
diff --git a/block/qcow2.c b/block/qcow2.c
index 4897aba..27e5a2c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4367,14 +4367,20 @@  static int qcow2_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
-static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
+static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
+                                                  Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     ImageInfoSpecific *spec_info;
     QCryptoBlockInfo *encrypt_info = NULL;
+    Error *local_err = NULL;
 
     if (s->crypto != NULL) {
-        encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);
+        encrypt_info = qcrypto_block_get_info(s->crypto, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return NULL;
+        }
     }
 
     spec_info = g_new(ImageInfoSpecific, 1);
diff --git a/block/vmdk.c b/block/vmdk.c
index 2c9e86d..544c10d 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2314,7 +2314,8 @@  static int coroutine_fn vmdk_co_check(BlockDriverState *bs,
     return ret;
 }
 
-static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
+static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs,
+                                                 Error **errp)
 {
     int i;
     BDRVVmdkState *s = bs->opaque;
diff --git a/include/block/block.h b/include/block/block.h
index f70a843..9899c24 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -477,7 +477,8 @@  const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
-ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
+ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
+                                          Error **errp);
 void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t offset, int64_t bytes,
                             int64_t *cluster_offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622..0075baf 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -319,7 +319,8 @@  struct BlockDriver {
                                   const char *name,
                                   Error **errp);
     int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi);
-    ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs);
+    ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs,
+                                                 Error **errp);
 
     int coroutine_fn (*bdrv_save_vmstate)(BlockDriverState *bs,
                                           QEMUIOVector *qiov,
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2c39124..2187036 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1661,6 +1661,7 @@  static int info_f(BlockBackend *blk, int argc, char **argv)
     BlockDriverState *bs = blk_bs(blk);
     BlockDriverInfo bdi;
     ImageInfoSpecific *spec_info;
+    Error *local_err = NULL;
     char s1[64], s2[64];
     int ret;
 
@@ -1682,7 +1683,11 @@  static int info_f(BlockBackend *blk, int argc, char **argv)
     printf("cluster size: %s\n", s1);
     printf("vm state offset: %s\n", s2);
 
-    spec_info = bdrv_get_specific_info(bs);
+    spec_info = bdrv_get_specific_info(bs, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -EIO;
+    }
     if (spec_info) {
         printf("Format specific information:\n");
         bdrv_image_info_specific_dump(fprintf, stdout, spec_info);