[v4,06/38] block: Make bdrv_is_inserted() recursive

If bdrv_is_inserted() is called on the top level BDS, it should make
sure all nodes in the BDS tree are actually inserted.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Am 20.07.2015 um 19:45 hat Max Reitz geschrieben:
> If bdrv_is_inserted() is called on the top level BDS, it should make
> sure all nodes in the BDS tree are actually inserted.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 494e08e..1d27b6a 100644
> --- a/block.c
> +++ b/block.c
> @@ -3235,10 +3235,9 @@ bool bdrv_is_inserted(BlockDriverState *bs)
>      if (!drv) {
>          return false;
>      }
> -    if (!drv->bdrv_is_inserted) {
> -        return true;
> -    }
> -    return drv->bdrv_is_inserted(bs);
> +    return (!drv->bdrv_is_inserted || drv->bdrv_is_inserted(bs)) &&
> +           (!bs->file              || bdrv_is_inserted(bs->file)) &&
> +           (!bs->backing_hd        || bdrv_is_inserted(bs->backing_hd));
>  }

Hm... Recursion often makes the right semantics unclear. I think though
what you're after here is good as a default behaviour, i.e. a non-leaf
node is inserted iff all of its children are inserted. We can do things
in various ways without breaking stuff because raw-posix is the only
driver actually implementing .bdrv_is_inserted, but I think it would
make most sense like this:

* If a driver implements .bdrv_is_inserted, we use this (and only this)
  for determining whether a medium is inserted.

* The default behaviour for drivers which don't have .bdrv_is_inserted
  is checking all children (in bs->children, not restricted to file and
  backing_hd, so that quorum etc. work)

* Consequently, a driver that doesn't want all of its children
  considered (which may be a very valid desire), can implement its own
  handler and doesn't get the default handling then.

This seems to be the most consistent with other recursive operations.

Also, after this patch, raw_bsd.c should be able drop its
implementation.

Kevin

On 07.09.2015 19:43, Kevin Wolf wrote:
> Am 20.07.2015 um 19:45 hat Max Reitz geschrieben:
>> If bdrv_is_inserted() is called on the top level BDS, it should make
>> sure all nodes in the BDS tree are actually inserted.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 494e08e..1d27b6a 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3235,10 +3235,9 @@ bool bdrv_is_inserted(BlockDriverState *bs)
>>      if (!drv) {
>>          return false;
>>      }
>> -    if (!drv->bdrv_is_inserted) {
>> -        return true;
>> -    }
>> -    return drv->bdrv_is_inserted(bs);
>> +    return (!drv->bdrv_is_inserted || drv->bdrv_is_inserted(bs)) &&
>> +           (!bs->file              || bdrv_is_inserted(bs->file)) &&
>> +           (!bs->backing_hd        || bdrv_is_inserted(bs->backing_hd));
>>  }
> 
> Hm... Recursion often makes the right semantics unclear. I think though
> what you're after here is good as a default behaviour, i.e. a non-leaf
> node is inserted iff all of its children are inserted. We can do things
> in various ways without breaking stuff because raw-posix is the only
> driver actually implementing .bdrv_is_inserted, but I think it would
> make most sense like this:
> 
> * If a driver implements .bdrv_is_inserted, we use this (and only this)
>   for determining whether a medium is inserted.
> 
> * The default behaviour for drivers which don't have .bdrv_is_inserted
>   is checking all children (in bs->children, not restricted to file and
>   backing_hd, so that quorum etc. work)
> 
> * Consequently, a driver that doesn't want all of its children
>   considered (which may be a very valid desire), can implement its own
>   handler and doesn't get the default handling then.
> 
> This seems to be the most consistent with other recursive operations.

You're right, I'll change this patch accordingly.

> Also, after this patch, raw_bsd.c should be able drop its
> implementation.

Indeed.

Max