[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [PATCH v2 04/33] block: Add BdrvChildRole to BdrvChild
Date: Thu, 6 Feb 2020 11:53:02 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

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 <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
> 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?


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]