Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers

[Max Reitz]

On 26.03.20 02:12, Maxim Levitsky wrote:
> Instead of checking the .bdrv_co_create_opts to see if we need the failback,
> just implement the .bdrv_co_create_opts in the drivers that need it.
> 
> This way we don't break various places that need to know if the underlying
> protocol/format really supports image creation, and this way we still
> allow some drivers to not support image creation.
> 
> Fixes: fd17146cd93d1704cd96d7c2757b325fc7aac6fd
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1816007

Thanks, the series looks good to me, just some thoughts below.

> Note that technically this driver reverts the image creation failback
> for the vxhs driver since I don't have a means to test it,
> and IMHO it is better to leave it not supported as it was prior to
> generic image creation patches.

There’s also file-win32.  I don’t really have the means to test that
either, though, so I suppose it’s reasonable to wait until someone
requests it.  OTOH, it shouldn’t be different from file-posix, so maybe
it wouldn’t hurt to support it, too.

We could also take this series for 5.0 as-is, and queue a file-win32
patch for 5.1.

What do you think?

> Also drop iscsi_create_opts which was left accidently
> 
> Signed-off-by: Maxim Levitsky <address@hidden>
> ---
>  block.c               | 35 ++++++++++++++++++++---------------
>  block/file-posix.c    |  7 ++++++-
>  block/iscsi.c         | 16 ++++------------
>  block/nbd.c           |  6 ++++++
>  block/nvme.c          |  3 +++
>  include/block/block.h |  7 +++++++
>  6 files changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ff23e20443..72fdf56af7 100644
> --- a/block.c
> +++ b/block.c
> @@ -598,8 +598,15 @@ static int 
> create_file_fallback_zero_first_sector(BlockBackend *blk,
>      return 0;
>  }
>  
> -static int bdrv_create_file_fallback(const char *filename, BlockDriver *drv,
> -                                     QemuOpts *opts, Error **errp)
> +/**
> + * Simple implementation of bdrv_co_create_opts for protocol drivers
> + * which only support creation via opening a file
> + * (usually existing raw storage device)
> + */
> +int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
> +                                           const char *filename,
> +                                           QemuOpts *opts,
> +                                           Error **errp)

The alignment’s off (in the header, too), but that can be fixed when
this series is applied.

>  {
>      BlockBackend *blk;
>      QDict *options;
> @@ -663,11 +670,7 @@ int bdrv_create_file(const char *filename, QemuOpts 
> *opts, Error **errp)
>          return -ENOENT;
>      }
>  
> -    if (drv->bdrv_co_create_opts) {
> -        return bdrv_create(drv, filename, opts, errp);
> -    } else {
> -        return bdrv_create_file_fallback(filename, drv, opts, errp);
> -    }
> +    return bdrv_create(drv, filename, opts, errp);

I thought we’d just let the drivers set BlockDriver.create_opts to
&bdrv_create_opts_simple and keep this bit of code (maybe with an
“else if (drv->create_opts != NULL)” and an
“assert(drv->create_opts == &bdrv_create_opts_simple)”).  That would
make the first patch unnecessary.

OTOH, it’s completely reasonable to pass the object as the first
argument to a class method, so why not.  (Well, technically the
BlockDriver kind of is the class, and the BDS is the object, it’s
weird.)  And it definitely follows what we do elsewhere (to provide
default implementations for block drivers to use).

>  }
>  
>  int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)

[...]

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 65bc980bc4..7e19bbff5f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c

[...]

> @@ -3639,10 +3641,11 @@ static BlockDriver bdrv_host_cdrom = {
>      .bdrv_reopen_prepare = raw_reopen_prepare,
>      .bdrv_reopen_commit  = raw_reopen_commit,
>      .bdrv_reopen_abort   = raw_reopen_abort,
> +    .bdrv_co_create_opts = bdrv_co_create_opts_simple,
> +    .create_opts         = &bdrv_create_opts_simple,
>      .mutable_opts        = mutable_opts,
>      .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
>  
> -

This line removal seems unrelated, but why not.

Max

>      .bdrv_co_preadv         = raw_co_preadv,
>      .bdrv_co_pwritev        = raw_co_pwritev,
>      .bdrv_co_flush_to_disk  = raw_co_flush_to_disk,

signature.asc
Description: OpenPGP digital signature