Re: [PATCH v5 07/45] block: document connection between child roles and bs->backing/bs->file

[Hanna Reitz <hreitz@redhat.com>]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> Make the informal rules formal. In further commit we'll add
> corresponding assertions.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   include/block/block-common.h | 42 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index fdb7306e78..2687a2519c 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -313,6 +313,48 @@ enum {
>    *
>    * At least one of DATA, METADATA, FILTERED, or COW must be set for
>    * every child.
> + *
> + *
> + * = Connection with bs->children, bs->file and bs->backing fields =
> + *
> + * 1. Filters
> + *
> + * Filter drivers has drv->is_filter = true.

s/has/have/

> + *
> + * Filter driver has exactly one FILTERED|PRIMARY child, any may have other

s/Filter driver/A filter node/?

And s/any/and/, I think.

> + * children which must not have these bits (the example is copy-before-write
> + * filter that also has target DATA child).

Mild style suggestion: “one example is the copy-before write filter, 
which also has its target DATA child”

> + *
> + * Filter driver never has COW children.

Maybe “Filter nodes never have COW children.”?

> + *
> + * For all filters except for mirror_top and commit_top, the filtered child is
> + * linked in bs->file, bs->backing is NULL.
> + *
> + * For mirror_top and commit_top filtered child is linked in bs->backing and

s/commit_top filtered/commit_top, the filtered/ (like in the paragraph 
above)

> + * their bs->file is NULL. These two filters has drv->filtered_child_is_backing

s/has/have/

> + * = true.

This also applies to the two test drivers in test-bdrv-graph-mod; should 
that be mentioned, too?

Or should we just link to filtered_child_is_backing when it comes to the 
list of drivers for which this applies, e.g. by rephrasing the two 
paragraphs as follows:

For most filters, the filtered child is linked in bs->file, bs->backing 
is NULL.  For some filters (as an exception), it is the other way 
around; those drivers will have drv->filtered_child_is_backing set to 
true (see that field’s documentation for what drivers this concerns).

(Just so we don’t duplicate the list of drivers.)

> + *
> + * 2. "raw" driver (block/raw-format.c)
> + *
> + * Formally it's not a filter (drv->is_filter = false)
> + *
> + * bs->backing is always NULL
> + *
> + * Only has one child, linked in bs->file. It's role is either FILTERED|PRIMARY

s/it's/its/

> + * (like filter) either DATA|PRIMARY depending on options.

s/either/or/

> + *
> + * 3. Other drivers
> + *
> + * Doesn't have any FILTERED children.

s/Doesn't/Don't/ (because “drivers” was in plural)

> + *
> + * May have at most one COW child. In this case it's linked in bs->backing.
> + * Otherwise bs->backing is NULL. COW child is never PRIMARY.
> + *
> + * May have at most one PRIMARY child. In this case it's linked in bs->file.
> + * Otherwise bs->file is NULL.
> + *
> + * May also have some other children that don't have neither PRIMARY nor COW
> + * bits set.

I think either “that don't have the PRIMARY or COW bit set" or "that 
have neither the PRIMARY nor the COW bit set".

>    */
>   enum BdrvChildRoleBits {
>       /*

Aside from typo/style nit picks, sounds good!