diff mbox

[v8,11/15] vmdk: Return extent's file in bdrv_get_block_status

Message ID 1453689887-2567-12-git-send-email-famz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fam Zheng Jan. 25, 2016, 2:44 a.m. UTC
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/vmdk.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Kevin Wolf Jan. 25, 2016, 1:28 p.m. UTC | #1
Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/vmdk.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index e1d3e27..f8f7fcf 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1274,6 +1274,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
>                               0, 0);
>      qemu_co_mutex_unlock(&s->lock);
>  
> +    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
>      switch (ret) {
>      case VMDK_ERROR:
>          ret = -EIO;
> @@ -1286,14 +1287,15 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
>          break;
>      case VMDK_OK:
>          ret = BDRV_BLOCK_DATA;
> -        if (extent->file == bs->file && !extent->compressed) {
> -            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
> +        if (!extent->compressed) {
> +            ret |= BDRV_BLOCK_OFFSET_VALID;
> +            ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
> +                    & BDRV_BLOCK_OFFSET_MASK;
>          }
> -
> +        *file = extent->file->bs;
>          break;
>      }
>  
> -    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
>      n = extent->cluster_sectors - index_in_cluster;
>      if (n > nb_sectors) {
>          n = nb_sectors;

The commit message made me expect that this does exactly the same as the
other patches, i.e. fill the file argument and nothing else.

Instead, this extends the functioniality to work on any kind of extents
instead of just the the main VMDK file, and it changes the calculation
of the offset (seems to be a hidden bug fix?)

This needs a non-empty commit message, and depending on what you're
going to write there, possibly splitting the patch.

Kevin
Fam Zheng Jan. 26, 2016, 3:44 a.m. UTC | #2
On Mon, 01/25 14:28, Kevin Wolf wrote:
> Am 25.01.2016 um 03:44 hat Fam Zheng geschrieben:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/vmdk.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/vmdk.c b/block/vmdk.c
> > index e1d3e27..f8f7fcf 100644
> > --- a/block/vmdk.c
> > +++ b/block/vmdk.c
> > @@ -1274,6 +1274,7 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
> >                               0, 0);
> >      qemu_co_mutex_unlock(&s->lock);
> >  
> > +    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
> >      switch (ret) {
> >      case VMDK_ERROR:
> >          ret = -EIO;
> > @@ -1286,14 +1287,15 @@ static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
> >          break;
> >      case VMDK_OK:
> >          ret = BDRV_BLOCK_DATA;
> > -        if (extent->file == bs->file && !extent->compressed) {
> > -            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
> > +        if (!extent->compressed) {
> > +            ret |= BDRV_BLOCK_OFFSET_VALID;
> > +            ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
> > +                    & BDRV_BLOCK_OFFSET_MASK;
> >          }
> > -
> > +        *file = extent->file->bs;
> >          break;
> >      }
> >  
> > -    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
> >      n = extent->cluster_sectors - index_in_cluster;
> >      if (n > nb_sectors) {
> >          n = nb_sectors;
> 
> The commit message made me expect that this does exactly the same as the
> other patches, i.e. fill the file argument and nothing else.
> 
> Instead, this extends the functioniality to work on any kind of extents
> instead of just the the main VMDK file, and it changes the calculation
> of the offset (seems to be a hidden bug fix?)
> 
> This needs a non-empty commit message, and depending on what you're
> going to write there, possibly splitting the patch.

Will split it.

Fam
diff mbox

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index e1d3e27..f8f7fcf 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1274,6 +1274,7 @@  static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
                              0, 0);
     qemu_co_mutex_unlock(&s->lock);
 
+    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
     switch (ret) {
     case VMDK_ERROR:
         ret = -EIO;
@@ -1286,14 +1287,15 @@  static int64_t coroutine_fn vmdk_co_get_block_status(BlockDriverState *bs,
         break;
     case VMDK_OK:
         ret = BDRV_BLOCK_DATA;
-        if (extent->file == bs->file && !extent->compressed) {
-            ret |= BDRV_BLOCK_OFFSET_VALID | offset;
+        if (!extent->compressed) {
+            ret |= BDRV_BLOCK_OFFSET_VALID;
+            ret |= (offset + (index_in_cluster << BDRV_SECTOR_BITS))
+                    & BDRV_BLOCK_OFFSET_MASK;
         }
-
+        *file = extent->file->bs;
         break;
     }
 
-    index_in_cluster = vmdk_find_index_in_cluster(extent, sector_num);
     n = extent->cluster_sectors - index_in_cluster;
     if (n > nb_sectors) {
         n = nb_sectors;