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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2787 bytes --]

On 05.02.20 17:48, Eric Blake wrote:
> 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'.

It could or maybe even should, but that would make the comments more
awkward to place.

Locally, at some point I had two bools that were just set to the result
of the conditions, and then I used them in an if/else if tree, but that
wasn’t any less awkward.

>> +    {
>> +        flags |= BDRV_O_PROTOCOL;
>> +    }
>> +
> 
> Looks good!  Lots of decision trees, but also lots of good comments
> backing up that complexity.

OK :-)

Max


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