[2/5] block: add helper function to determine if a BDS is in a chain

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(+)

The Wednesday 14 May 2014 à 23:20:16 (-0400), 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(+)
> 
> diff --git a/block.c b/block.c
> index 81945d3..c4f77c2 100644
> --- a/block.c
> +++ b/block.c
> @@ -3734,6 +3734,15 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
>      return NULL;
>  }
>  
> +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
> +{
> +    while (top && top != base) {
> +        top = top->backing_hd;
> +    }
> +
> +    return top != NULL;
> +}
> +
>  BlockDriverState *bdrv_next(BlockDriverState *bs)
>  {
>      if (!bs) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 1b119aa..283a6f3 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -381,6 +381,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);
>  BlockDriverState *bdrv_lookup_bs(const char *device,
>                                   const char *node_name,
>                                   Error **errp);
> +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base);
>  BlockDriverState *bdrv_next(BlockDriverState *bs);
>  void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
>                    void *opaque);
> -- 
> 1.8.3.1
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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.

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 <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.

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

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 <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.
> 

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.
>