|
From: | Eric Blake |
Subject: | Re: [PATCH v2 04/33] block: Add BdrvChildRole to BdrvChild |
Date: | Tue, 11 Feb 2020 09:37:28 -0600 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 |
On 2/6/20 4:53 AM, Max Reitz wrote:
+++ 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.
Yeah, the self-documenting aspect is nice.
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?
You're right that we haven't done it, but it is also the slickest solution that preserves documentation intent. In C, such a typedef serves only as documentation (and the compiler doesn't enforce it, compared to what a strongly-typed language would do), but I do like the idea.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[Prev in Thread] | Current Thread | [Next in Thread] |