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. The function itself looks useful, though, once we agree on the naming. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
