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 <ebl...@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 <stefa...@redhat.com> Signed-off-by: Manos Pitsidianakis <el13...@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 <ebl...@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.
signature.asc
Description: PGP signature