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 <vsement...@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;
      }