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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

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 <jcody@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]