[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 41/56] block/copy-before-write: make public block driver
From: |
Kevin Wolf |
Subject: |
Re: [PULL 41/56] block/copy-before-write: make public block driver |
Date: |
Mon, 20 Sep 2021 11:41:58 +0200 |
Am 01.09.2021 um 17:16 hat Hanna Reitz geschrieben:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Finally, copy-before-write gets own .bdrv_open and .bdrv_close
> handlers, block_init() call and becomes available through bdrv_open().
>
> To achieve this:
>
> - cbw_init gets unused flags argument and becomes cbw_open
> - block_copy_state_free() call moved to new cbw_close()
> - in bdrv_cbw_append:
> - options are completed with driver and node-name, and we can simply
> use bdrv_insert_node() to do both open and drained replacing
> - in bdrv_cbw_drop:
> - cbw_close() is now responsible for freeing s->bcs, so don't do it
> here
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Message-Id: <20210824083856.17408-22-vsementsov@virtuozzo.com>
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
This patch results in a change in behaviour that I assume was not
intentional: Previously, creating a backup block job would succeed to
insert the filter node independently of whether the filter driver was
whitelisted or not. After this patch, it becomes an error if the filter
driver isn't explicitly whitelisted:
https://bugzilla.redhat.com/show_bug.cgi?id=2004812
> block/copy-before-write.c | 60 ++++++++++++++++++---------------------
> 1 file changed, 28 insertions(+), 32 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 2efe098aae..2cd68b480a 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -144,7 +144,8 @@ static void cbw_child_perm(BlockDriverState *bs,
> BdrvChild *c,
> }
> }
>
> -static int cbw_init(BlockDriverState *bs, QDict *options, Error **errp)
> +static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
> + Error **errp)
> {
> BDRVCopyBeforeWriteState *s = bs->opaque;
> BdrvDirtyBitmap *copy_bitmap;
> @@ -181,10 +182,21 @@ static int cbw_init(BlockDriverState *bs, QDict
> *options, Error **errp)
> return 0;
> }
>
> +static void cbw_close(BlockDriverState *bs)
> +{
> + BDRVCopyBeforeWriteState *s = bs->opaque;
> +
> + block_copy_state_free(s->bcs);
> + s->bcs = NULL;
> +}
> +
> BlockDriver bdrv_cbw_filter = {
> .format_name = "copy-before-write",
> .instance_size = sizeof(BDRVCopyBeforeWriteState),
>
> + .bdrv_open = cbw_open,
> + .bdrv_close = cbw_close,
> +
> .bdrv_co_preadv = cbw_co_preadv,
> .bdrv_co_pwritev = cbw_co_pwritev,
> .bdrv_co_pwrite_zeroes = cbw_co_pwrite_zeroes,
> @@ -205,56 +217,40 @@ BlockDriverState *bdrv_cbw_append(BlockDriverState
> *source,
> Error **errp)
> {
> ERRP_GUARD();
> - int ret;
> BDRVCopyBeforeWriteState *state;
> BlockDriverState *top;
> QDict *opts;
>
> assert(source->total_sectors == target->total_sectors);
>
> - top = bdrv_new_open_driver(&bdrv_cbw_filter, filter_node_name,
> - BDRV_O_RDWR, errp);
bdrv_new_open_driver() is a relatively short and straightforward code
path that just directly opens a driver for internal use.
> - if (!top) {
> - error_prepend(errp, "Cannot open driver: ");
> - return NULL;
> - }
> - state = top->opaque;
> -
> opts = qdict_new();
> + qdict_put_str(opts, "driver", "copy-before-write");
> + if (filter_node_name) {
> + qdict_put_str(opts, "node-name", filter_node_name);
> + }
> qdict_put_str(opts, "file", bdrv_get_node_name(source));
> qdict_put_str(opts, "target", bdrv_get_node_name(target));
>
> - ret = cbw_init(top, opts, errp);
> - qobject_unref(opts);
> - if (ret < 0) {
> - goto fail;
> - }
> -
> - bdrv_drained_begin(source);
> - ret = bdrv_replace_node(source, top, errp);
> - bdrv_drained_end(source);
> - if (ret < 0) {
> - error_prepend(errp, "Cannot append copy-before-write filter: ");
> - goto fail;
> + top = bdrv_insert_node(source, opts, BDRV_O_RDWR, errp);
On the other hand, bdrv_insert_node() uses bdrv_open() internally, which
is a really long and messy code path that basically has to handle all of
the craziness that compatibility code for -drive needs to have.
Do we really need bdrv_open() here?
Or is all you really need just a version of bdrv_new_open_driver() that
accepts an options QDict instead of just flags?
Kevin
- [PULL 31/56] block/copy-before-write: drop extra bdrv_unref on failure path, (continued)
- [PULL 31/56] block/copy-before-write: drop extra bdrv_unref on failure path, Hanna Reitz, 2021/09/01
- [PULL 30/56] block/copy-before-write: relax permission requirements when no parents, Hanna Reitz, 2021/09/01
- [PULL 33/56] block/copy-before-write: bdrv_cbw_append(): replace child at last, Hanna Reitz, 2021/09/01
- [PULL 38/56] block/copy-before-write: cbw_init(): use options, Hanna Reitz, 2021/09/01
- [PULL 36/56] block/copy-before-write: cbw_init(): use file child after attaching, Hanna Reitz, 2021/09/01
- [PULL 37/56] block/copy-before-write: bdrv_cbw_append(): drop unused compress arg, Hanna Reitz, 2021/09/01
- [PULL 35/56] block/copy-before-write: cbw_init(): rename variables, Hanna Reitz, 2021/09/01
- [PULL 39/56] block/copy-before-write: initialize block-copy bitmap, Hanna Reitz, 2021/09/01
- [PULL 40/56] block/block-copy: make setting progress optional, Hanna Reitz, 2021/09/01
- [PULL 41/56] block/copy-before-write: make public block driver, Hanna Reitz, 2021/09/01
- Re: [PULL 41/56] block/copy-before-write: make public block driver,
Kevin Wolf <=
- [PULL 43/56] python/qemu/machine.py: refactor _qemu_args(), Hanna Reitz, 2021/09/01
- [PULL 42/56] qapi: publish copy-before-write filter, Hanna Reitz, 2021/09/01
- [PULL 44/56] python/qemu/machine: QEMUMachine: improve qmp() method, Hanna Reitz, 2021/09/01
- [PULL 45/56] python:QEMUMachine: template typing for self returning methods, Hanna Reitz, 2021/09/01
- [PULL 46/56] iotests/222: fix pylint and mypy complains, Hanna Reitz, 2021/09/01
- [PULL 47/56] iotests/222: constantly use single quotes for strings, Hanna Reitz, 2021/09/01
- [PULL 48/56] iotests: move 222 to tests/image-fleecing, Hanna Reitz, 2021/09/01
- [PULL 50/56] iotests/image-fleecing: proper source device, Hanna Reitz, 2021/09/01
- [PULL 49/56] iotests.py: hmp_qemu_io: support qdev, Hanna Reitz, 2021/09/01