[Qemu-devel] [PATCH] vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info

[Qemu-devel] [PATCH] vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info

[Fam Zheng]

When extent types don't match, we return -ENOTSUP. In this case, be
polite to the caller and don't modify bdi.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 673d3f5..2cbfd3e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2137,23 +2137,29 @@ static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
     return spec_info;
 }
 
+static bool vmdk_extents_type_eq(const VmdkExtent *a, const VmdkExtent *b)
+{
+    return a->flat == b->flat &&
+           a->compressed == b->compressed &&
+           (a->flat || a->cluster_sectors == b->cluster_sectors);
+}
+
 static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     int i;
     BDRVVmdkState *s = bs->opaque;
     assert(s->num_extents);
-    bdi->needs_compressed_writes = s->extents[0].compressed;
-    if (!s->extents[0].flat) {
-        bdi->cluster_size = s->extents[0].cluster_sectors << BDRV_SECTOR_BITS;
-    }
+
     /* See if we have multiple extents but they have different cases */
     for (i = 1; i < s->num_extents; i++) {
-        if (bdi->needs_compressed_writes != s->extents[i].compressed ||
-            (bdi->cluster_size && bdi->cluster_size !=
-                s->extents[i].cluster_sectors << BDRV_SECTOR_BITS)) {
+        if (!vmdk_extents_type_eq(&s->extents[0], &s->extents[i])) {
             return -ENOTSUP;
         }
     }
+    bdi->needs_compressed_writes = s->extents[0].compressed;
+    if (!s->extents[0].flat) {
+        bdi->cluster_size = s->extents[0].cluster_sectors << BDRV_SECTOR_BITS;
+    }
     return 0;
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info

[Max Reitz]

On 2014-11-14 at 05:09, Fam Zheng wrote:
> When extent types don't match, we return -ENOTSUP. In this case, be
> polite to the caller and don't modify bdi.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/vmdk.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH] vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

On Fri, Nov 14, 2014 at 12:09:21PM +0800, Fam Zheng wrote:
> When extent types don't match, we return -ENOTSUP. In this case, be
> polite to the caller and don't modify bdi.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)

At this stage of the release process it would be very helpful to include
some context for this patch: is it a pure cleanup or actually a bug fix
that needs to go into QEMU 2.2?

Since you are a regular contributor and I therefore trust you, I have
merged this patch.  But please be clear in commit descriptions about
whether or not a patch is a bug fix (how to reproduce the bug, if a
previous commit's regression is being fixed, etc).

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] vmdk: Leave bdi intact if -ENOTSUP in vmdk_get_info

[Fam Zheng]

On Fri, 11/14 09:29, Stefan Hajnoczi wrote:
> On Fri, Nov 14, 2014 at 12:09:21PM +0800, Fam Zheng wrote:
> > When extent types don't match, we return -ENOTSUP. In this case, be
> > polite to the caller and don't modify bdi.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/vmdk.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> At this stage of the release process it would be very helpful to include
> some context for this patch: is it a pure cleanup or actually a bug fix
> that needs to go into QEMU 2.2?

This one is more of a cleanup than bug fix: although there is a small logical
change, it's only visible to user on abnormally structured VMDK (mixed extent
type, etc.) that require manual editing image file, which is AFAIK not a valid
use case and will not cause anything worse than an error. I sent the patch
mostly for the function semantic purpose: don't change caller's data unless
it's necessary to.

> 
> Since you are a regular contributor and I therefore trust you, I have
> merged this patch.  But please be clear in commit descriptions about
> whether or not a patch is a bug fix (how to reproduce the bug, if a
> previous commit's regression is being fixed, etc).

OK. I'll remember next time. Thanks for pointing out.

Fam