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

[Eric Blake <eblake@redhat.com>]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]