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

[Eric Blake]

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