Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
On 26.03.20 14:35, Eric Blake wrote:
> On 3/26/20 8:28 AM, Kevin Wolf wrote:
>> Am 26.03.2020 um 14:20 hat Eric Blake geschrieben:
+++ b/block/file-posix.c
@@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
.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 = _create_opts_simple,
>>>
>>> I'd drop the leading & for consistency with the rest of this struct
>>> initializer.
>>
>> This one isn't a function pointer, so I think the & is necessary.
>
> Ah, right. Visual pattern-matching failed me, since I didn't read the
> actual types in the .h file.
>
> Hmm - is it possible to write the patch in such a way that .create_opts
> can be left NULL when .bdrv_co_create_opts is bdrv_co_create_opts_simple?
Setting .create_opts is actually the main point of this series, so we
don’t have to look for and fix all places that decide whether a block
driver is capable of image creation based on whether it’s set or not.
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
On 3/26/20 8:28 AM, Kevin Wolf wrote:
Am 26.03.2020 um 14:20 hat Eric Blake geschrieben:
+++ b/block/file-posix.c
@@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
.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 = _create_opts_simple,
I'd drop the leading & for consistency with the rest of this struct
initializer.
This one isn't a function pointer, so I think the & is necessary.
Ah, right. Visual pattern-matching failed me, since I didn't read the
actual types in the .h file.
Hmm - is it possible to write the patch in such a way that .create_opts
can be left NULL when .bdrv_co_create_opts is bdrv_co_create_opts_simple?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
On Thu, 2020-03-26 at 08:20 -0500, Eric Blake wrote:
> On 3/25/20 8:12 PM, Maxim Levitsky wrote:
> > Instead of checking the .bdrv_co_create_opts to see if we need the failback,
>
> fallback
100% true.
>
> > 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
> >
> > Note that technically this driver reverts the image creation failback
>
> fallback
>
> > 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.
> >
> > Also drop iscsi_create_opts which was left accidently
>
> accidentally
True. I did a spell check on the commit message, but I guess I updated it
afterward with this.
>
> >
> > Signed-off-by: Maxim Levitsky
> > ---
> > +++ b/block/file-posix.c
> > @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
> > .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 = _create_opts_simple,
>
> I'd drop the leading & for consistency with the rest of this struct
> initializer.
Can I? This is struct reference and I think that only for function references,
the leading & is optional.
Best regards,
Maxim Levitsky
Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
Am 26.03.2020 um 14:20 hat Eric Blake geschrieben:
> > +++ b/block/file-posix.c
> > @@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
> > .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 = _create_opts_simple,
>
> I'd drop the leading & for consistency with the rest of this struct
> initializer.
This one isn't a function pointer, so I think the & is necessary.
Kevin
Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
On 3/25/20 8:12 PM, Maxim Levitsky wrote:
Instead of checking the .bdrv_co_create_opts to see if we need the failback,
fallback
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
Note that technically this driver reverts the image creation failback
fallback
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.
Also drop iscsi_create_opts which was left accidently
accidentally
Signed-off-by: Maxim Levitsky
---
+++ b/block/file-posix.c
@@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
.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 = _create_opts_simple,
I'd drop the leading & for consistency with the rest of this struct
initializer.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
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
> ---
> 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
_create_opts_simple and keep this bit of code (maybe with an
“else if (drv->create_opts != NULL)” and an
“assert(drv->create_opts == _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 = _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
[PATCH 2/2] block: trickle down the fallback image creation function use to the block drivers
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
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.
Also drop iscsi_create_opts which was left accidently
Signed-off-by: Maxim Levitsky
---
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)
{
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);
}
int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
@@ -1592,9 +1595,9 @@ QemuOptsList bdrv_runtime_opts = {
},
};
-static QemuOptsList fallback_create_opts = {
-.name = "fallback-create-opts",
-.head = QTAILQ_HEAD_INITIALIZER(fallback_create_opts.head),
+QemuOptsList bdrv_create_opts_simple = {
+.name = "simple-create-opts",
+.head = QTAILQ_HEAD_INITIALIZER(bdrv_create_opts_simple.head),
.desc = {
{
.name = BLOCK_OPT_SIZE,
@@ -5963,13 +5966,15 @@ void bdrv_img_create(const char *filename, const char
*fmt,
return;
}
+if (!proto_drv->create_opts) {
+error_setg(errp, "Protocol driver '%s' does not support image
creation",
+ proto_drv->format_name);
+return;
+}
+
/* Create parameter list */
create_opts = qemu_opts_append(create_opts, drv->create_opts);
-if (proto_drv->create_opts) {
-create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
-} else {
-create_opts = qemu_opts_append(create_opts, _create_opts);
-}
+create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
opts = qemu_opts_create(create_opts, NULL, 0, _abort);
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
@@ -3513,6 +3513,8 @@ static BlockDriver bdrv_host_device = {
.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 = _create_opts_simple,
.mutable_opts= mutable_opts,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
.bdrv_co_pwrite_zeroes = hdev_co_pwrite_zeroes,
@@ -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 = _create_opts_simple,
.mutable_opts= mutable_opts,
.bdrv_co_invalidate_cache = raw_co_invalidate_cache,
-
.bdrv_co_preadv = raw_co_preadv,
.bdrv_co_pwritev= raw_co_pwritev,
.bdrv_co_flush_to_disk = raw_co_flush_to_disk,
@@ -3771,6 +3774,8 @@ 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 = _create_opts_simple,
.mutable_opts
