Re: [PATCH v2 22/33] block: Make backing files child_of_bds children

[Eric Blake]

On 2/4/20 11:08 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <address@hidden>

Another sparse commit message (a recurring theme of this series). The 
subject line says 'what', and the patch appears to be faithful to that, 
but if a future bisection lands here, even a one-sentence 'why' would be 
handy; maybe:

This is part of a larger series of unifying block device relationships 
via child_of_bds.

to at least hint that searching nearby commits gives a better why.

> ---
>   block.c                 | 26 ++++++++++++++++++++------
>   block/backup-top.c      |  2 +-
>   block/vvfat.c           |  3 ++-
>   tests/test-bdrv-drain.c | 13 +++++++------
>   4 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/block.c b/block.c
> index 77755f0c6c..6b705ee23a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2748,6 +2748,20 @@ static bool 
> bdrv_inherits_from_recursive(BlockDriverState *child,
>       return child != NULL;
>   }
>   
> +/*
> + * Return the BdrvChildRole for @bs's backing child.  bs->backing is
> + * mostly used for COW backing children (role = COW), but also for
> + * filtered children (role = FILTERED | PRIMARY).
> + */
> +static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
> +{
> +    if (bs->drv && bs->drv->is_filter) {
> +        return BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY;

And here's the first point (that I've spotted at least) in this series 
where you are definitely returning a non-BdrvChildRole through a return 
type of BdrvChildRole, rather than me just guessing you might (the 
integer formed by bitwise-or of two enum values is not itself an enum 
value).  Repeating what I said earlier, the C language is loose enough 
to allow your usage, and your usage is somewhat better self-documenting 
than using an unsigned int; but it would not fly in other languages.

So I won't insist you change it, but at least think about it. (And the 
latter has already happened if you read my paragraph - so can we call 
that enough thought on the matter? ;)

> +    } else {
> +        return BDRV_CHILD_COW;
> +    }
> +}
> +
Reviewed-by: Eric Blake <address@hidden>

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