1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v5 0/4] block: Block driver callbacks fixes
  4. View patch

[Qemu-devel] [PATCH v5 1/4] block: pass bdrv_* methods to bs->file by default in block filters

Manos Pitsidianakis posted 4 patches 8 years, 7 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v5 1/4] block: pass bdrv_* methods to bs->file by default in block filters
Posted by Manos Pitsidianakis 8 years, 7 months ago 
The following functions fail if bs->drv is a filter and does not
implement them:

bdrv_probe_blocksizes
bdrv_probe_geometry
bdrv_truncate
bdrv_has_zero_init
bdrv_get_info

Instead, the call should be passed to bs->file if it exists, to allow
filter drivers to support those methods without implementing them. This
commit makes `drv->is_filter = true` imply that these callbacks will be
forwarded to bs->file by default, so disabling support for these
functions must be done explicitly.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c                   | 21 +++++++++++++++++++--
 include/block/block_int.h |  6 +++++-
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index d24ae85868..892f29d231 100644
--- a/block.c
+++ b/block.c
@@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 
     if (drv && drv->bdrv_probe_blocksizes) {
         return drv->bdrv_probe_blocksizes(bs, bsz);
+    } else if (drv && drv->is_filter && bs->file) {
+        return bdrv_probe_blocksizes(bs->file->bs, bsz);
     }
 
     return -ENOTSUP;
@@ -511,6 +513,8 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 
     if (drv && drv->bdrv_probe_geometry) {
         return drv->bdrv_probe_geometry(bs, geo);
+    } else if (drv && drv->is_filter && bs->file) {
+        return bdrv_probe_geometry(bs->file->bs, geo);
     }
 
     return -ENOTSUP;
@@ -3420,11 +3424,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
 
     assert(child->perm & BLK_PERM_RESIZE);
 
+    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
     if (!drv) {
         error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
     }
     if (!drv->bdrv_truncate) {
+        if (bs->file && drv->is_filter) {
+            return bdrv_truncate(bs->file, prealloc, offset, errp);
+        }
         error_setg(errp, "Image format driver does not support resize");
         return -ENOTSUP;
     }
@@ -3761,6 +3769,9 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     if (bs->drv->bdrv_has_zero_init) {
         return bs->drv->bdrv_has_zero_init(bs);
     }
+    if (bs->file && bs->drv->is_filter) {
+        return bdrv_has_zero_init(bs->file->bs);
+    }
 
     /* safe default */
     return 0;
@@ -3815,10 +3826,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
+    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
+    if (!drv) {
         return -ENOMEDIUM;
-    if (!drv->bdrv_get_info)
+    }
+    if (!drv->bdrv_get_info) {
+        if (bs->file && drv->is_filter) {
+            return bdrv_get_info(bs->file->bs, bdi);
+        }
         return -ENOTSUP;
+    }
     memset(bdi, 0, sizeof(*bdi));
     return drv->bdrv_get_info(bs, bdi);
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 669a2797fd..37388ccb18 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -87,7 +87,11 @@ struct BlockDriver {
     const char *format_name;
     int instance_size;
 
-    /* set to true if the BlockDriver is a block filter */
+    /* set to true if the BlockDriver is a block filter. Block filters pass
+     * certain callbacks that refer to data (see block.c) to their bs->file if
+     * the driver doesn't implement them. Drivers that do not wish to forward
+     * must implement them and return -ENOTSUP.
+     */
     bool is_filter;
     /* for snapshots block filter like Quorum can implement the
      * following recursive callback.
-- 
2.11.0
Re: [Qemu-devel] [PATCH v5 1/4] block: pass bdrv_* methods to bs->file by default in block filters
Posted by Eric Blake 8 years, 7 months ago 
On 07/13/2017 05:01 AM, Manos Pitsidianakis wrote:
> The following functions fail if bs->drv is a filter and does not
> implement them:
> 
> bdrv_probe_blocksizes
> bdrv_probe_geometry
> bdrv_truncate
> bdrv_has_zero_init
> bdrv_get_info
> 
> Instead, the call should be passed to bs->file if it exists, to allow
> filter drivers to support those methods without implementing them. This
> commit makes `drv->is_filter = true` imply that these callbacks will be
> forwarded to bs->file by default, so disabling support for these
> functions must be done explicitly.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

I would have dropped R-b on the grounds that you added comments to
'is_filter' (good! but non-trivial), to make sure it gets a
grammar/wording review of the new content.  Furthermore, your patch had
a semantic change due to the rebase, and looking at that change...

> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---

> @@ -3420,11 +3424,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
>  
>      assert(child->perm & BLK_PERM_RESIZE);
>  
> +    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
>      if (!drv) {
>          error_setg(errp, "No medium inserted");
>          return -ENOMEDIUM;
>      }
>      if (!drv->bdrv_truncate) {
> +        if (bs->file && drv->is_filter) {
> +            return bdrv_truncate(bs->file, prealloc, offset, errp);

...ouch - you got it wrong.  s/prealloc, offset/offset, prealloc/

> +++ b/include/block/block_int.h
> @@ -87,7 +87,11 @@ struct BlockDriver {
>      const char *format_name;
>      int instance_size;
>  
> -    /* set to true if the BlockDriver is a block filter */
> +    /* set to true if the BlockDriver is a block filter. Block filters pass
> +     * certain callbacks that refer to data (see block.c) to their bs->file if
> +     * the driver doesn't implement them. Drivers that do not wish to forward
> +     * must implement them and return -ENOTSUP.
> +     */
>      bool is_filter;

I'm okay with the new comment.

With the swapped parameter order fixed (the maintainer can probably do
that),
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v5 1/4] block: pass bdrv_* methods to bs->file by default in block filters
Posted by Manos Pitsidianakis 8 years, 7 months ago 
On Thu, Jul 13, 2017 at 06:23:14AM -0500, Eric Blake wrote:
>On 07/13/2017 05:01 AM, Manos Pitsidianakis wrote:
>> The following functions fail if bs->drv is a filter and does not
>> implement them:
>>
>> bdrv_probe_blocksizes
>> bdrv_probe_geometry
>> bdrv_truncate
>> bdrv_has_zero_init
>> bdrv_get_info
>>
>> Instead, the call should be passed to bs->file if it exists, to allow
>> filter drivers to support those methods without implementing them. This
>> commit makes `drv->is_filter = true` imply that these callbacks will be
>> forwarded to bs->file by default, so disabling support for these
>> functions must be done explicitly.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>I would have dropped R-b on the grounds that you added comments to
>'is_filter' (good! but non-trivial), to make sure it gets a
>grammar/wording review of the new content.  Furthermore, your patch had
>a semantic change due to the rebase, and looking at that change...
>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> ---
>
>> @@ -3420,11 +3424,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, PreallocMode prealloc,
>>
>>      assert(child->perm & BLK_PERM_RESIZE);
>>
>> +    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
>>      if (!drv) {
>>          error_setg(errp, "No medium inserted");
>>          return -ENOMEDIUM;
>>      }
>>      if (!drv->bdrv_truncate) {
>> +        if (bs->file && drv->is_filter) {
>> +            return bdrv_truncate(bs->file, prealloc, offset, errp);
>
>...ouch - you got it wrong.  s/prealloc, offset/offset, prealloc/
>
>> +++ b/include/block/block_int.h
>> @@ -87,7 +87,11 @@ struct BlockDriver {
>>      const char *format_name;
>>      int instance_size;
>>
>> -    /* set to true if the BlockDriver is a block filter */
>> +    /* set to true if the BlockDriver is a block filter. Block filters pass
>> +     * certain callbacks that refer to data (see block.c) to their bs->file if
>> +     * the driver doesn't implement them. Drivers that do not wish to forward
>> +     * must implement them and return -ENOTSUP.
>> +     */
>>      bool is_filter;
>
>I'm okay with the new comment.
>
>With the swapped parameter order fixed (the maintainer can probably do
>that),
>Reviewed-by: Eric Blake <eblake@redhat.com>

Oh, this compiled correctly because PreallocMode is an enum. Sorry for 
the inconvenience, that was rushed. I will send a new version to be 
typical.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.