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

[Fam Zheng]

On 2013年11月14日 09:34, Jeff Cody wrote:
> 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 <address@hidden>
> > > > ---
> > > >   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.

We care for -ENOTSUP as well as other err code in the compressed case, 
and don't care the call to bdrv_get_info in the other case. I get your 
point, I'll add "else if (ret != -ENOTSUP) {...}" below the call, and 
keep the check of ret in "if (compressed)" unchanged.

Fam