[PATCH 3/4] block: Add blk_make_empty()

Max Reitz posted 4 patches 5 years, 9 months ago
Maintainers: Xie Changlong <xiechanglong.d@gmail.com>, Max Reitz <mreitz@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Wen Congyang <wencongyang2@huawei.com>, John Snow <jsnow@redhat.com>
There is a newer version of this series
[PATCH 3/4] block: Add blk_make_empty()
Posted by Max Reitz 5 years, 9 months ago
Two callers of BlockDriver.bdrv_make_empty() remain that should not call
this method directly.  Both do not have access to a BdrvChild, but they
can use a BlockBackend, so we add this function that lets them use it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/sysemu/block-backend.h | 2 ++
 block/block-backend.c          | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index d37c1244dd..14338b76dc 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
 
 const BdrvChild *blk_root(BlockBackend *blk);
 
+int blk_make_empty(BlockBackend *blk, Error **errp);
+
 #endif
diff --git a/block/block-backend.c b/block/block-backend.c
index 3592066b42..5d36efd32f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
 {
     return blk->root;
 }
+
+int blk_make_empty(BlockBackend *blk, Error **errp)
+{
+    return bdrv_make_empty(blk->root, errp);
+}
-- 
2.25.4


Re: [PATCH 3/4] block: Add blk_make_empty()
Posted by Eric Blake 5 years, 9 months ago
On 4/28/20 8:26 AM, Max Reitz wrote:
> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
> this method directly.  Both do not have access to a BdrvChild, but they
> can use a BlockBackend, so we add this function that lets them use it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   include/sysemu/block-backend.h | 2 ++
>   block/block-backend.c          | 5 +++++
>   2 files changed, 7 insertions(+)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index d37c1244dd..14338b76dc 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>   
>   const BdrvChild *blk_root(BlockBackend *blk);
>   
> +int blk_make_empty(BlockBackend *blk, Error **errp);
> +

Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice.

>   #endif
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3592066b42..5d36efd32f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>   {
>       return blk->root;
>   }
> +
> +int blk_make_empty(BlockBackend *blk, Error **errp)
> +{
> +    return bdrv_make_empty(blk->root, errp);
> +}
> 

Otherwise looks fine.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH 3/4] block: Add blk_make_empty()
Posted by Eric Blake 5 years, 9 months ago
On 4/28/20 8:55 AM, Eric Blake wrote:

>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend 
>> *blk_in, int64_t off_in,
>>   const BdrvChild *blk_root(BlockBackend *blk);
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
> 
> Again, a flag parameter to allow use of BDRV_REQ_NO_FALLBACK would be nice.

Or maybe not, after reading Kevin's responses.  Making an image empty is 
not the same as making it read as zero.  If we can't come up with a use 
for a flag, then deferring the addition of a flag until later is a 
perfectly reasonable approach (rather than adding a flag now that will 
never get set to anything other than 0).  This isn't quite the same as a 
public API where we would regret being locked out of a flag down the road.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH 3/4] block: Add blk_make_empty()
Posted by Kevin Wolf 5 years, 9 months ago
Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
> this method directly.  Both do not have access to a BdrvChild, but they
> can use a BlockBackend, so we add this function that lets them use it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/sysemu/block-backend.h | 2 ++
>  block/block-backend.c          | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index d37c1244dd..14338b76dc 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>  
>  const BdrvChild *blk_root(BlockBackend *blk);
>  
> +int blk_make_empty(BlockBackend *blk, Error **errp);
> +
>  #endif
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3592066b42..5d36efd32f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>  {
>      return blk->root;
>  }
> +
> +int blk_make_empty(BlockBackend *blk, Error **errp)
> +{
> +    return bdrv_make_empty(blk->root, errp);
> +}

Should we check that blk->root != NULL? Most other functions do that
through blk_is_available().

Kevin


Re: [PATCH 3/4] block: Add blk_make_empty()
Posted by Max Reitz 5 years, 9 months ago
On 28.04.20 16:47, Kevin Wolf wrote:
> Am 28.04.2020 um 15:26 hat Max Reitz geschrieben:
>> Two callers of BlockDriver.bdrv_make_empty() remain that should not call
>> this method directly.  Both do not have access to a BdrvChild, but they
>> can use a BlockBackend, so we add this function that lets them use it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  include/sysemu/block-backend.h | 2 ++
>>  block/block-backend.c          | 5 +++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index d37c1244dd..14338b76dc 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -266,4 +266,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>>  
>>  const BdrvChild *blk_root(BlockBackend *blk);
>>  
>> +int blk_make_empty(BlockBackend *blk, Error **errp);
>> +
>>  #endif
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 3592066b42..5d36efd32f 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2402,3 +2402,8 @@ const BdrvChild *blk_root(BlockBackend *blk)
>>  {
>>      return blk->root;
>>  }
>> +
>> +int blk_make_empty(BlockBackend *blk, Error **errp)
>> +{
>> +    return bdrv_make_empty(blk->root, errp);
>> +}
> 
> Should we check that blk->root != NULL? Most other functions do that
> through blk_is_available().

Why not.

Max