We are going to add one more option of enum type. Let's refactor option
parsing so that we can simply work with BlockdevOptionsCbw object.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
---
block/copy-before-write.c | 68 +++++++++++++++++++++++----------------
1 file changed, 41 insertions(+), 27 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index a8a06fdc09..394e73b094 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/qmp/qjson.h"
#include "sysemu/block-backend.h"
#include "qemu/cutils.h"
@@ -328,46 +329,49 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
}
}
-static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
- Error **errp)
+static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
{
- QDict *bitmap_qdict = NULL;
- BlockDirtyBitmap *bmp_param = NULL;
+ QDict *cbw_qdict = NULL;
+ BlockdevOptionsCbw *opts = NULL;
Visitor *v = NULL;
- bool ret = false;
- *bitmap = NULL;
+ cbw_qdict = qdict_clone_shallow(options);
- qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
- if (!qdict_size(bitmap_qdict)) {
- ret = true;
- goto out;
- }
-
- v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
+ /*
+ * Delete BlockdevOptions base fields, that are not part of
+ * BlockdevOptionsCbw.
+ */
+ qdict_del(cbw_qdict, "driver");
+ qdict_del(cbw_qdict, "node-name");
+ qdict_del(cbw_qdict, "discard");
+ qdict_del(cbw_qdict, "cache");
+ qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
+ qdict_del(cbw_qdict, "read-only");
+ qdict_del(cbw_qdict, "auto-read-only");
+ qdict_del(cbw_qdict, "force-share");
+ qdict_del(cbw_qdict, "detect-zeroes");
+
+ v = qobject_input_visitor_new_flat_confused(cbw_qdict, errp);
if (!v) {
goto out;
}
- visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
- if (!bmp_param) {
- goto out;
- }
-
- *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
- errp);
- if (!*bitmap) {
+ visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
+ if (!opts) {
goto out;
}
- ret = true;
+ /*
+ * Delete options which we are going to parse through BlockdevOptionsCbw
+ * object for original options.
+ */
+ qdict_extract_subqdict(options, NULL, "bitmap");
out:
- qapi_free_BlockDirtyBitmap(bmp_param);
visit_free(v);
- qobject_unref(bitmap_qdict);
+ qobject_unref(cbw_qdict);
- return ret;
+ return opts;
}
static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
@@ -376,6 +380,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
BDRVCopyBeforeWriteState *s = bs->opaque;
BdrvDirtyBitmap *bitmap = NULL;
int64_t cluster_size;
+ g_autoptr(BlockdevOptionsCbw) opts = NULL;
+
+ opts = cbw_parse_options(options, errp);
+ if (!opts) {
+ return -EINVAL;
+ }
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -390,8 +400,12 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
- if (!cbw_parse_bitmap_option(options, &bitmap, errp)) {
- return -EINVAL;
+ if (opts->has_bitmap) {
+ bitmap = block_dirty_bitmap_lookup(opts->bitmap->node,
+ opts->bitmap->name, NULL, errp);
+ if (!bitmap) {
+ return -EINVAL;
+ }
}
bs->total_sectors = bs->file->bs->total_sectors;
--
2.35.1On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
> We are going to add one more option of enum type. Let's refactor option
> parsing so that we can simply work with BlockdevOptionsCbw object.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
> block/copy-before-write.c | 68 +++++++++++++++++++++++----------------
> 1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index a8a06fdc09..394e73b094 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -24,6 +24,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/qmp/qjson.h"
>
> #include "sysemu/block-backend.h"
> #include "qemu/cutils.h"
> @@ -328,46 +329,49 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
> }
> }
>
> -static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
> - Error **errp)
> +static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
> {
> - QDict *bitmap_qdict = NULL;
> - BlockDirtyBitmap *bmp_param = NULL;
> + QDict *cbw_qdict = NULL;
> + BlockdevOptionsCbw *opts = NULL;
> Visitor *v = NULL;
> - bool ret = false;
>
> - *bitmap = NULL;
> + cbw_qdict = qdict_clone_shallow(options);
>
> - qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
> - if (!qdict_size(bitmap_qdict)) {
> - ret = true;
> - goto out;
> - }
> -
> - v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
> + /*
> + * Delete BlockdevOptions base fields, that are not part of
> + * BlockdevOptionsCbw.
> + */
> + qdict_del(cbw_qdict, "driver");
> + qdict_del(cbw_qdict, "node-name");
> + qdict_del(cbw_qdict, "discard");
> + qdict_del(cbw_qdict, "cache");
> + qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
> + qdict_del(cbw_qdict, "read-only");
> + qdict_del(cbw_qdict, "auto-read-only");
> + qdict_del(cbw_qdict, "force-share");
> + qdict_del(cbw_qdict, "detect-zeroes");
Works in practice now, but seems a bit fragile. If new fields are added
to the base class, this will break. (And I don’t know whether people
will think of updating this when new fields are added to the base class.)
Would there be a problem if instead we parsed the full BlockdevOptions
object here, asserting that .driver is BLOCKDEV_DRIVER_COPY_BEFORE_WRITE?
> +
> + v = qobject_input_visitor_new_flat_confused(cbw_qdict, errp);
> if (!v) {
> goto out;
> }
>
> - visit_type_BlockDirtyBitmap(v, NULL, &bmp_param, errp);
> - if (!bmp_param) {
> - goto out;
> - }
> -
> - *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
> - errp);
> - if (!*bitmap) {
> + visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
> + if (!opts) {
> goto out;
> }
01.04.2022 13:50, Hanna Reitz wrote:
> On 01.04.22 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> We are going to add one more option of enum type. Let's refactor option
>> parsing so that we can simply work with BlockdevOptionsCbw object.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
>> ---
>> block/copy-before-write.c | 68 +++++++++++++++++++++++----------------
>> 1 file changed, 41 insertions(+), 27 deletions(-)
>>
>> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
>> index a8a06fdc09..394e73b094 100644
>> --- a/block/copy-before-write.c
>> +++ b/block/copy-before-write.c
>> @@ -24,6 +24,7 @@
>> */
>> #include "qemu/osdep.h"
>> +#include "qapi/qmp/qjson.h"
>> #include "sysemu/block-backend.h"
>> #include "qemu/cutils.h"
>> @@ -328,46 +329,49 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
>> }
>> }
>> -static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
>> - Error **errp)
>> +static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
>> {
>> - QDict *bitmap_qdict = NULL;
>> - BlockDirtyBitmap *bmp_param = NULL;
>> + QDict *cbw_qdict = NULL;
>> + BlockdevOptionsCbw *opts = NULL;
>> Visitor *v = NULL;
>> - bool ret = false;
>> - *bitmap = NULL;
>> + cbw_qdict = qdict_clone_shallow(options);
>> - qdict_extract_subqdict(options, &bitmap_qdict, "bitmap.");
>> - if (!qdict_size(bitmap_qdict)) {
>> - ret = true;
>> - goto out;
>> - }
>> -
>> - v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
>> + /*
>> + * Delete BlockdevOptions base fields, that are not part of
>> + * BlockdevOptionsCbw.
>> + */
>> + qdict_del(cbw_qdict, "driver");
>> + qdict_del(cbw_qdict, "node-name");
>> + qdict_del(cbw_qdict, "discard");
>> + qdict_del(cbw_qdict, "cache");
>> + qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
>> + qdict_del(cbw_qdict, "read-only");
>> + qdict_del(cbw_qdict, "auto-read-only");
>> + qdict_del(cbw_qdict, "force-share");
>> + qdict_del(cbw_qdict, "detect-zeroes");
>
> Works in practice now, but seems a bit fragile. If new fields are added to the base class, this will break. (And I don’t know whether people will think of updating this when new fields are added to the base class.)
>
> Would there be a problem if instead we parsed the full BlockdevOptions object here, asserting that .driver is BLOCKDEV_DRIVER_COPY_BEFORE_WRITE?
Hmm. Thanks! Yes there is a problem with it, as some options are already absorbed in bdrv_open_common(). But good news is that the only necessary absorbed option is "driver".
So, instead of asserting .driver, we temporary add "driver"="copy-before-write" into options, and then we can natively parse them as BlockdevOptions.
The following works for me (applied on the top of the series), will merge into v3:
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0ea5506f77..3d6c9fc055 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -375,40 +375,25 @@ static void cbw_child_perm(BlockDriverState *bs, BdrvChild *c,
}
}
-static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
+static BlockdevOptions *cbw_parse_options(QDict *options, Error **errp)
{
- QDict *cbw_qdict = NULL;
- BlockdevOptionsCbw *opts = NULL;
+ BlockdevOptions *opts = NULL;
Visitor *v = NULL;
- cbw_qdict = qdict_clone_shallow(options);
+ qdict_put_str(options, "driver", "copy-before-write");
- /*
- * Delete BlockdevOptions base fields, that are not part of
- * BlockdevOptionsCbw.
- */
- qdict_del(cbw_qdict, "driver");
- qdict_del(cbw_qdict, "node-name");
- qdict_del(cbw_qdict, "discard");
- qdict_del(cbw_qdict, "cache");
- qdict_extract_subqdict(cbw_qdict, NULL, "cache.");
- qdict_del(cbw_qdict, "read-only");
- qdict_del(cbw_qdict, "auto-read-only");
- qdict_del(cbw_qdict, "force-share");
- qdict_del(cbw_qdict, "detect-zeroes");
-
- v = qobject_input_visitor_new_flat_confused(cbw_qdict, errp);
+ v = qobject_input_visitor_new_flat_confused(options, errp);
if (!v) {
goto out;
}
- visit_type_BlockdevOptionsCbw(v, NULL, &opts, errp);
+ visit_type_BlockdevOptions(v, NULL, &opts, errp);
if (!opts) {
goto out;
}
/*
- * Delete options which we are going to parse through BlockdevOptionsCbw
+ * Delete options which we are going to parse through BlockdevOptions
* object for original options.
*/
qdict_extract_subqdict(options, NULL, "bitmap");
@@ -417,7 +402,7 @@ static BlockdevOptionsCbw *cbw_parse_options(QDict *options, Error **errp)
out:
visit_free(v);
- qobject_unref(cbw_qdict);
+ qdict_del(options, "driver");
return opts;
}
@@ -428,12 +413,14 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
BDRVCopyBeforeWriteState *s = bs->opaque;
BdrvDirtyBitmap *bitmap = NULL;
int64_t cluster_size;
- g_autoptr(BlockdevOptionsCbw) opts = NULL;
+ g_autoptr(BlockdevOptions) full_opts = NULL;
+ BlockdevOptionsCbw *opts;
- opts = cbw_parse_options(options, errp);
- if (!opts) {
+ full_opts = cbw_parse_options(options, errp);
+ if (!full_opts) {
return -EINVAL;
}
+ opts = &full_opts->u.copy_before_write;
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.