| Message ID | 20200326011218.29230-3-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix the generic image creation code | expand |
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,
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 <mlevitsk@redhat.com> > --- > +++ 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.
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
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 <mlevitsk@redhat.com> > > --- > > +++ 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
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?
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
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, &fallback_create_opts); - } + create_opts = qemu_opts_append(create_opts, proto_drv->create_opts); opts = qemu_opts_create(create_opts, NULL, 0, &error_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 = &bdrv_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 = &bdrv_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 = &bdrv_create_opts_simple, .mutable_opts = mutable_opts, .bdrv_co_preadv = raw_co_preadv, diff --git a/block/iscsi.c b/block/iscsi.c index 682abd8e09..14680a436a 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -2399,18 +2399,6 @@ out_unlock: return r; } -static QemuOptsList iscsi_create_opts = { - .name = "iscsi-create-opts", - .head = QTAILQ_HEAD_INITIALIZER(iscsi_create_opts.head), - .desc = { - { - .name = BLOCK_OPT_SIZE, - .type = QEMU_OPT_SIZE, - .help = "Virtual disk size" - }, - { /* end of list */ } - } -}; static const char *const iscsi_strong_runtime_opts[] = { "transport", @@ -2434,6 +2422,8 @@ static BlockDriver bdrv_iscsi = { .bdrv_parse_filename = iscsi_parse_filename, .bdrv_file_open = iscsi_open, .bdrv_close = iscsi_close, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, @@ -2471,6 +2461,8 @@ static BlockDriver bdrv_iser = { .bdrv_parse_filename = iscsi_parse_filename, .bdrv_file_open = iscsi_open, .bdrv_close = iscsi_close, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_reopen_prepare = iscsi_reopen_prepare, .bdrv_reopen_commit = iscsi_reopen_commit, .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache, diff --git a/block/nbd.c b/block/nbd.c index 976be76647..2160859f64 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -2038,6 +2038,8 @@ static BlockDriver bdrv_nbd = { .protocol_name = "nbd", .instance_size = sizeof(BDRVNBDState), .bdrv_parse_filename = nbd_parse_filename, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_file_open = nbd_open, .bdrv_reopen_prepare = nbd_client_reopen_prepare, .bdrv_co_preadv = nbd_client_co_preadv, @@ -2063,6 +2065,8 @@ static BlockDriver bdrv_nbd_tcp = { .protocol_name = "nbd+tcp", .instance_size = sizeof(BDRVNBDState), .bdrv_parse_filename = nbd_parse_filename, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_file_open = nbd_open, .bdrv_reopen_prepare = nbd_client_reopen_prepare, .bdrv_co_preadv = nbd_client_co_preadv, @@ -2088,6 +2092,8 @@ static BlockDriver bdrv_nbd_unix = { .protocol_name = "nbd+unix", .instance_size = sizeof(BDRVNBDState), .bdrv_parse_filename = nbd_parse_filename, + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, .bdrv_file_open = nbd_open, .bdrv_reopen_prepare = nbd_client_reopen_prepare, .bdrv_co_preadv = nbd_client_co_preadv, diff --git a/block/nvme.c b/block/nvme.c index d41c4bda6e..7b7c0cc5d6 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -1333,6 +1333,9 @@ static BlockDriver bdrv_nvme = { .protocol_name = "nvme", .instance_size = sizeof(BDRVNVMeState), + .bdrv_co_create_opts = bdrv_co_create_opts_simple, + .create_opts = &bdrv_create_opts_simple, + .bdrv_parse_filename = nvme_parse_filename, .bdrv_file_open = nvme_file_open, .bdrv_close = nvme_close, diff --git a/include/block/block.h b/include/block/block.h index e569a4d747..7d36ec5433 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -283,6 +283,13 @@ BlockDriver *bdrv_find_format(const char *format_name); int bdrv_create(BlockDriver *drv, const char* filename, QemuOpts *opts, Error **errp); int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp); + +int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv, + const char *filename, + QemuOpts *opts, + Error **errp); +extern QemuOptsList bdrv_create_opts_simple; + BlockDriverState *bdrv_new(void); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp);
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 <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(-)