Re: [Qemu-devel] [PATCH 1/5] qemu-img: Convert by cluster size if target is compressed

[Jeff Cody <jcody@redhat.com>]

On Thu, Nov 14, 2013 at 09:18:35AM +0800, Fam Zheng wrote:
> On 2013年11月13日 23:23, Jeff Cody wrote:
> >On Wed, Nov 13, 2013 at 08:53:10AM +0800, Fam Zheng wrote:
> >>If target block driver forces compression, qemu-img convert needs to
> >>write by cluster size as well as "-c" option.
> >>
> >>Particularly, this applies for converting to VMDK streamOptimized
> >>format.
> >>
> >>Signed-off-by: Fam Zheng <famz@redhat.com>
> >>---
> >>  include/block/block.h | 1 +
> >>  qemu-img.c            | 5 ++++-
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/include/block/block.h b/include/block/block.h
> >>index 3560deb..169c092 100644
> >>--- a/include/block/block.h
> >>+++ b/include/block/block.h
> >>@@ -18,6 +18,7 @@ typedef struct BlockDriverInfo {
> >>      /* offset at which the VM state can be saved (0 if not possible) */
> >>      int64_t vm_state_offset;
> >>      bool is_dirty;
> >>+    bool is_compressed;
> >>  } BlockDriverInfo;
> >>
> >>  typedef struct BlockFragInfo {
> >>diff --git a/qemu-img.c b/qemu-img.c
> >>index bf3fb4f..09ed9b2 100644
> >>--- a/qemu-img.c
> >>+++ b/qemu-img.c
> >>@@ -1383,8 +1383,11 @@ static int img_convert(int argc, char **argv)
> >>          }
> >>      }
> >>
> >>+    ret = bdrv_get_info(out_bs, &bdi);
> >>+    if (ret == 0) {
> >>+        compress = compress || bdi.is_compressed;
> >>+    }
> >>      if (compress) {
> >>-        ret = bdrv_get_info(out_bs, &bdi);
> >>          if (ret < 0) {
> >>              error_report("could not get block driver info");
> >>              goto out;
> >
> >You need to move the error checking up as well, above the
> >'if (compress)'
> >
> 
> Moving it up forces bdrv_get_info to succeed for both compress case
> and non compress case, which is too strict and fails when target
> driver doesn't support it.

bdrv_get_info() will return -ENOTSUP if it is not supported.  If we do
call down to the driver layer bdrv_get_info() and get an error, I
think we should pass it up.  So just ignore -ENOTSUP if we don't care
about the case when bdrv_get_info() is not supported by a target.

> 
> I see it's just odd to assign ret outside if block and check it in
> only one branch, but I don't have a better way to do this here.
> 
> Fam
>