Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain

[Kevin Wolf]

Am 15.05.2014 um 16:16 hat Eric Blake geschrieben:
> On 05/14/2014 09:20 PM, Jeff Cody wrote:
> > This is a small helper function, to determine if 'base' is in the
> > chain of BlockDriverState 'top'.  It returns true if it is in the chain,
> > and false otherwise.
> > 
> > If either argument is NULL, it will also return false.
> > 
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  block.c               | 9 +++++++++
> >  include/block/block.h | 1 +
> >  2 files changed, 10 insertions(+)
> > 
> 
> >  
> > +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
> 
> No doc comments inline, and not everyone has the commit message handy.
> Which means someone trying to learn what this command does has to read
> the function.  Maybe copy the commit message into code comments as well.
> 
> Bikeshedding: If I'm reading code, and see bdrv_is_in_chain(a, b), my
> first inclination would be to read that as "return true if node a is in
> chain b".  But if I were to see bdrv_chain_contains(a, b), I would parse
> that as "return true if chain a contains node b".  I think you either
> want to swap argument order, or rename the function.

I haven't read the patch yet, so I can't say much about the context, but
just from seeing the function names I think I agree with you.

Kevin

pgpYhfyQd_LPE.pgp
Description: PGP signature