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 <mre...@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
signature.asc
Description: OpenPGP digital signature
