On Thu, May 15, 2014 at 08:16:27AM -0600, Eric Blake wrote: > 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. >
OK > 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 like the bdrv_chain_contains() as the function name, it is clearer. I'll use that. > > The function itself looks useful, though, once we agree on the naming. >
