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

[Eric Blake]

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

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