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

[Vladimir Sementsov-Ogievskiy]

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