qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 1/4] block: pass bdrv_* methods to bs->file b


From: Manos Pitsidianakis
Subject: Re: [Qemu-devel] [PATCH v5 1/4] block: pass bdrv_* methods to bs->file by default in block filters
Date: Thu, 13 Jul 2017 14:39:30 +0300
User-agent: NeoMutt/20170609-57-1e93be (1.8.3)

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 <address@hidden>

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 <address@hidden>
Signed-off-by: Manos Pitsidianakis <address@hidden>
---

@@ -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 <address@hidden>

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.

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]