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 <mre...@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
signature.asc
Description: OpenPGP digital signature
