[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: Eric Blake
Subject: Re: [PATCH v2 04/33] block: Add BdrvChildRole to BdrvChild
Date: Wed, 5 Feb 2020 09:33:15 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1

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

reply via email to

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