[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 09/33] block: Add generic bdrv_inherited_options()
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 09/33] block: Add generic bdrv_inherited_options() |
Date: |
Thu, 6 Feb 2020 12:06:34 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
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 <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'.
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
- Re: [PATCH v2 03/33] block: Add BdrvChildRole, (continued)
- [PATCH v2 06/33] block: Pass BdrvChildRole to .inherit_options(), Max Reitz, 2020/02/04
- [PATCH v2 07/33] block: Pass parent_is_format to .inherit_options(), Max Reitz, 2020/02/04
- [PATCH v2 02/33] block: Rename BdrvChildRole to BdrvChildClass, Max Reitz, 2020/02/04
- [PATCH v2 09/33] block: Add generic bdrv_inherited_options(), Max Reitz, 2020/02/04
- [PATCH v2 08/33] block: Rename bdrv_inherited_options(), Max Reitz, 2020/02/04
- [PATCH v2 05/33] block: Pass BdrvChildRole to bdrv_child_perm(), Max Reitz, 2020/02/04
- [PATCH v2 04/33] block: Add BdrvChildRole to BdrvChild, Max Reitz, 2020/02/04
- [PATCH v2 12/33] block: Unify bdrv_child_cb_detach(), Max Reitz, 2020/02/04