[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
> (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
signature.asc
Description: OpenPGP digital signature
[PATCH v2 18/33] block: Add bdrv_default_perms(), Max Reitz, 2020/02/04
[PATCH v2 17/33] block: Split bdrv_default_perms_for_storage(), Max Reitz, 2020/02/04
[PATCH v2 16/33] block: Pull out bdrv_default_perms_for_storage(), Max Reitz, 2020/02/04
[PATCH v2 19/33] raw-format: Split raw_read_options(), Max Reitz, 2020/02/04