QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Maxim Levitsky <mlevitsk@redhat.com>, qemu-devel@nongnu.org
Cc: Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>,
	integration@gluster.org, sheepdog@lists.wpkg.org,
	Stefan Hajnoczi <stefanha@redhat.com>,
	qemu-block@nongnu.org, Jason Dillaman <dillaman@redhat.com>,
	Jeff Cody <codyprime@gmail.com>, Stefan Weil <sw@weilnetz.de>,
	Peter Lieven <pl@kamp.de>,
	"Richard W.M. Jones" <rjones@redhat.com>,
	"Denis V. Lunev" <den@openvz.org>,
	Ronnie Sahlberg <ronniesahlberg@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Liu Yuan <namei.unix@gmail.com>
Subject: Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
Date: Thu, 26 Mar 2020 12:20:45 +0100
Message-ID: <237a76c0-f447-6ba5-267d-b51da3ea2dd4@redhat.com> (raw)
In-Reply-To: <20200326011218.29230-3-mlevitsk@redhat.com>

[-- Attachment #1.1: Type: text/plain, Size: 4581 bytes --]

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 <mlevitsk@redhat.com>
> ---
>  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,


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-26  1:12 [PATCH 0/2] Fix the generic image creation code Maxim Levitsky
2020-03-26  1:12 ` [PATCH 1/2] block: pass BlockDriver reference to the .bdrv_co_create Maxim Levitsky
2020-03-26 13:18   ` Eric Blake
2020-03-26 13:22     ` Maxim Levitsky
2020-03-26 13:27       ` Eric Blake
2020-03-26  1:12 ` [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers Maxim Levitsky
2020-03-26 11:20   ` Max Reitz [this message]
2020-03-26 13:20   ` Eric Blake
2020-03-26 13:28     ` Kevin Wolf
2020-03-26 13:35       ` Eric Blake
2020-03-26 13:39         ` Max Reitz
2020-03-26 13:30     ` Maxim Levitsky
2020-03-26 10:55 ` [PATCH 0/2] Fix the generic image creation code Denis V. Lunev
2020-03-26 12:23 ` Max Reitz
2020-03-26 13:38   ` Max Reitz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=237a76c0-f447-6ba5-267d-b51da3ea2dd4@redhat.com \
    --to=mreitz@redhat.com \
    --cc=codyprime@gmail.com \
    --cc=den@openvz.org \
    --cc=dillaman@redhat.com \
    --cc=fam@euphon.net \
    --cc=integration@gluster.org \
    --cc=kwolf@redhat.com \
    --cc=mlevitsk@redhat.com \
    --cc=namei.unix@gmail.com \
    --cc=pbonzini@redhat.com \
    --cc=pl@kamp.de \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=rjones@redhat.com \
    --cc=ronniesahlberg@gmail.com \
    --cc=sheepdog@lists.wpkg.org \
    --cc=stefanha@redhat.com \
    --cc=sw@weilnetz.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git