Re: [PATCH 1/4] block: Add bdrv_make_empty()

[Eric Blake]

On 4/28/20 8:26 AM, Max Reitz wrote:
> Right now, all users of bdrv_make_empty() call the BlockDriver method
> directly.  That is not only bad style, it is also wrong, unless the
> caller has a BdrvChild with a WRITE permission.
>
> Introduce bdrv_make_empty() that verifies that it does.
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   include/block/block.h |  1 +
>   block.c               | 23 +++++++++++++++++++++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index b05995fe9c..d947fb4080 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -351,6 +351,7 @@ BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts 
> *opts,
>   void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
>   void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
>   int bdrv_commit(BlockDriverState *bs);
> +int bdrv_make_empty(BdrvChild *c, Error **errp);

Can we please fix this to take a flags parameter?  I want to make it 
easier for callers to request BDRV_REQ_NO_FALLBACK for distinguishing 
between callers where the image must be made empty (read as all zeroes) 
regardless of time spent, vs. made empty quickly (including if it is 
already all zero) but where the caller is prepared for the operation to 
fail and will write zeroes itself if fast bulk zeroing was not possible.


> +int bdrv_make_empty(BdrvChild *c, Error **errp)
> +{
> +    BlockDriver *drv = c->bs->drv;
> +    int ret;
> +
> +    assert(c->perm & BLK_PERM_WRITE);
> +
> +    if (!drv->bdrv_make_empty) {
> +        error_setg(errp, "%s does not support emptying nodes",
> +                   drv->format_name);
> +        return -ENOTSUP;
> +    }

And here's where we can add some automatic fallbacks, such as 
recognizing if the image already reads as all zeroes.  But those 
optimizations can come in separate patches; for YOUR patch, just getting 
the proper API in place is fine.

> +
> +    ret = drv->bdrv_make_empty(c->bs);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Failed to empty %s",
> +                         c->bs->filename);
> +        return ret;
> +    }
> +
> +    return 0;
> +}

Other than a missing flag parameter, this looks fine.


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