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 <jc...@redhat.com> > > --- > > 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
pgpyoaw1bjcyT.pgp
Description: PGP signature
