Re: [PATCH v2 04/33] block: Add BdrvChildRole to BdrvChild

[Eric Blake]

On 2/4/20 11:08 AM, Max Reitz wrote:
> For now, it is always set to 0.  Later patches in this series will
> ensure that all callers pass an appropriate combination of flags.

Sneaky - this re-adds the field you dropped as part of a rename in 2/33. 
 Anyone doing backport had better be aware that they would need this 
whole series, rather than cherry-picking later patches without the 
earlier ones.  But that observation does not affect the patch validity.

> Signed-off-by: Max Reitz <address@hidden>
> ---

> +++ b/block.c
> @@ -2381,6 +2381,7 @@ static void bdrv_replace_child(BdrvChild *child, 
> BlockDriverState *new_bs)
>   BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>                                     const char *child_name,
>                                     const BdrvChildClass *child_class,
> +                                  BdrvChildRole child_role,

Hmm - C is loose enough to allow the declaration of a parameter as an 
enum type even when its intended usage is to receive a bitwise-or 
derived from that enum but not declared in the enum.  For example, if I 
understand intent correctly, a caller might pass in 0x3 
(BDRV_CHILD_DATA|BDRV_CHILD_METADATA) which does NOT appear in 
BdrvChildRole.  It feels like it might be cleaner to document the 
interface as taking an unsigned int (although then you've lost the 
documentation that the int is derived from BdrvChildRole), than to abuse 
the typesystem to pass values that are not BdrvChildRole through the 
parameter.

Otherwise, this patch is a mechanical addition.
Reviewed-by: Eric Blake <address@hidden>

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