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 = &bdrv_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 = &bdrv_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 = &bdrv_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 = &bdrv_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 = &bdrv_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 &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