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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2214 bytes --]

On 05.02.20 16:33, Eric Blake wrote:
> 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 <mreitz@redhat.com>
>> ---
> 
>> +++ 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.

I don’t necessarily disagree, but we have pre-existing examples of such
abuse, like BdrvRequestFlags.

The advantage of using BdrvChildRole as a type here is to show that we
expect values from that enum.  I personally prefer that.

I mean, we could do something else entirely and name the enum
“BdrvChildRoleBits” and add a “typedef unsigned int BdrvChildRole;”.  I
don’t think we’ve ever done that before but maybe it’d be the cleanest way?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]