Re: [PATCH v2 09/33] block: Add generic bdrv_inherited_options()

From: Eric Blake <eblake@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v2 09/33] block: Add generic bdrv_inherited_options()
Date: Wed, 5 Feb 2020 10:48:27 -0600	[thread overview]
Message-ID: <87197a53-64e5-ec56-acff-13288256dee0@redhat.com> (raw)
In-Reply-To: <20200204170848.614480-10-mreitz@redhat.com>

On 2/4/20 11:08 AM, Max Reitz wrote:
> After the series this patch belongs to, we want to have a common
> BdrvChildClass that encompasses all of child_file, child_format, and
> child_backing.  Such a single class needs a single .inherit_options()
> implementation, and this patch introduces it.
> 
> The next patch will show how the existing implementations can fall back
> to it just by passing appropriate BdrvChildRole and parent_is_format
> values.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 84 insertions(+)
> 

No impact until the next patch, but the division of patches was wise.

> +    /*
> +     * First, decide whether to set, clear, or leave BDRV_O_PROTOCOL.
> +     * Generally, the question to answer is: Should this child be
> +     * format-probed by default?
> +     */
> +
> +    /*
> +     * Pure and non-filtered data children of non-format nodes should
> +     * be probed by default (even when the node itself has BDRV_O_PROTOCOL
> +     * set).  This only affects a very limited set of drivers (namely
> +     * quorum and blkverify when this comment was written).
> +     * Force-clear BDRV_O_PROTOCOL then.
> +     */
> +    if (!parent_is_format &&
> +        (role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
> +                 BDRV_CHILD_FILTERED)) ==
> +            BDRV_CHILD_DATA)
> +    {
> +        flags &= ~BDRV_O_PROTOCOL;
> +    }
> +
> +    /*
> +     * All children of format nodes (except for COW children) and all
> +     * metadata children in general should never be format-probed.
> +     * Force-set BDRV_O_PROTOCOL then.
> +     */
> +    if ((parent_is_format && !(role & BDRV_CHILD_COW)) ||
> +        (role & BDRV_CHILD_METADATA))

Should this use 'else if', to make it obvious that we never have a path 
that both force-clears and force-sets BDRV_O_PROTOCOL?  But a careful 
reading shows that the two 'if' are mutually exclusive, even without the 
second using 'else if'.

> +    {
> +        flags |= BDRV_O_PROTOCOL;
> +    }
> +

Looks good!  Lots of decision trees, but also lots of good comments 
backing up that complexity.
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org