Re: [PATCH v2 1/7] block/copy-before-write: refactor option parsing

[Hanna Reitz]

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?

> +
> +    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;
>       }