[Qemu-devel] [PATCH v2 08/13] block: Move flag inheritance to bdrv_open_inherit()

[Kevin Wolf]

Instead of letting every caller of bdrv_open() determine the right flags
for its child node manually and pass them to the function, pass the
parent node and the role of the newly opened child (like backing file,
protocol layer, etc.).

Signed-off-by: Kevin Wolf <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Max Reitz <address@hidden>
---
 block.c                   | 74 ++++++++++++++++++++++++++++++++++++++---------
 block/blkdebug.c          |  2 +-
 block/blkverify.c         |  4 +--
 block/quorum.c            |  4 +--
 block/vmdk.c              |  5 ++--
 include/block/block.h     |  4 ++-
 include/block/block_int.h |  7 +++++
 7 files changed, 78 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 7b466cf..7ba0edd 100644
--- a/block.c
+++ b/block.c
@@ -79,6 +79,12 @@ static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
 static QLIST_HEAD(, BlockDriver) bdrv_drivers =
     QLIST_HEAD_INITIALIZER(bdrv_drivers);
 
+static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
+                             const char *reference, QDict *options, int flags,
+                             BlockDriverState *parent,
+                             const BdrvChildRole *child_role,
+                             BlockDriver *drv, Error **errp);
+
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
@@ -682,8 +688,8 @@ static int bdrv_temp_snapshot_flags(int flags)
 }
 
 /*
- * Returns the flags that bs->file should get, based on the given flags for
- * the parent BDS
+ * Returns the flags that bs->file should get if a protocol driver is expected,
+ * based on the given flags for the parent BDS
  */
 static int bdrv_inherited_flags(int flags)
 {
@@ -700,6 +706,25 @@ static int bdrv_inherited_flags(int flags)
     return flags;
 }
 
+const BdrvChildRole child_file = {
+    .inherit_flags = bdrv_inherited_flags,
+};
+
+/*
+ * Returns the flags that bs->file should get if the use of formats (and not
+ * only protocols) is permitted for it, based on the given flags for the parent
+ * BDS
+ */
+static int bdrv_inherited_fmt_flags(int parent_flags)
+{
+    int flags = child_file.inherit_flags(parent_flags);
+    return flags & ~BDRV_O_PROTOCOL;
+}
+
+const BdrvChildRole child_format = {
+    .inherit_flags = bdrv_inherited_fmt_flags,
+};
+
 /*
  * Returns the flags that bs->backing_hd should get, based on the given flags
  * for the parent BDS
@@ -715,6 +740,10 @@ static int bdrv_backing_flags(int flags)
     return flags;
 }
 
+static const BdrvChildRole child_backing = {
+    .inherit_flags = bdrv_backing_flags,
+};
+
 static int bdrv_open_flags(BlockDriverState *bs, int flags)
 {
     int open_flags = flags | BDRV_O_CACHE_WB;
@@ -837,7 +866,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
         return 0;
     }
 
-    bs->open_flags = flags;
     bs->guest_block_size = 512;
     bs->request_alignment = 512;
     bs->zero_beyond_eof = true;
@@ -1147,9 +1175,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
     }
 
     assert(bs->backing_hd == NULL);
-    ret = bdrv_open(&backing_hd,
-                    *backing_filename ? backing_filename : NULL, NULL, options,
-                    bdrv_backing_flags(bs->open_flags), NULL, &local_err);
+    ret = bdrv_open_inherit(&backing_hd,
+                            *backing_filename ? backing_filename : NULL,
+                            NULL, options, 0, bs, &child_backing,
+                            NULL, &local_err);
     if (ret < 0) {
         bdrv_unref(backing_hd);
         backing_hd = NULL;
@@ -1183,7 +1212,8 @@ free_exit:
  * To conform with the behavior of bdrv_open(), *pbs has to be NULL.
  */
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
-                    QDict *options, const char *bdref_key, int flags,
+                    QDict *options, const char *bdref_key,
+                    BlockDriverState* parent, const BdrvChildRole *child_role,
                     bool allow_none, Error **errp)
 {
     QDict *image_options;
@@ -1211,7 +1241,8 @@ int bdrv_open_image(BlockDriverState **pbs, const char 
*filename,
         goto done;
     }
 
-    ret = bdrv_open(pbs, filename, reference, image_options, flags, NULL, 
errp);
+    ret = bdrv_open_inherit(pbs, filename, reference, image_options, 0,
+                            parent, child_role, NULL, errp);
 
 done:
     qdict_del(options, bdref_key);
@@ -1298,9 +1329,11 @@ out:
  * should be opened. If specified, neither options nor a filename may be given,
  * nor can an existing BDS be reused (that is, *pbs has to be NULL).
  */
-int bdrv_open(BlockDriverState **pbs, const char *filename,
-              const char *reference, QDict *options, int flags,
-              BlockDriver *drv, Error **errp)
+static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
+                             const char *reference, QDict *options, int flags,
+                             BlockDriverState *parent,
+                             const BdrvChildRole *child_role,
+                             BlockDriver *drv, Error **errp)
 {
     int ret;
     BlockDriverState *file = NULL, *bs;
@@ -1309,6 +1342,8 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
     int snapshot_flags = 0;
 
     assert(pbs);
+    assert(!child_role || !flags);
+    assert(!child_role == !parent);
 
     if (reference) {
         bool options_non_empty = options ? qdict_size(options) : false;
@@ -1346,6 +1381,10 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
         options = qdict_new();
     }
 
+    if (child_role) {
+        flags = child_role->inherit_flags(parent->open_flags);
+    }
+
     ret = bdrv_fill_options(&options, &filename, flags, drv, &local_err);
     if (local_err) {
         goto fail;
@@ -1371,6 +1410,7 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
         flags &= ~BDRV_O_PROTOCOL;
     }
 
+    bs->open_flags = flags;
     bs->options = options;
     options = qdict_clone_shallow(options);
 
@@ -1385,9 +1425,9 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
         }
 
         assert(file == NULL);
+        bs->open_flags = flags;
         ret = bdrv_open_image(&file, filename, options, "file",
-                              bdrv_inherited_flags(flags),
-                              true, &local_err);
+                              bs, &child_file, true, &local_err);
         if (ret < 0) {
             goto fail;
         }
@@ -1504,6 +1544,14 @@ close_and_fail:
     return ret;
 }
 
+int bdrv_open(BlockDriverState **pbs, const char *filename,
+              const char *reference, QDict *options, int flags,
+              BlockDriver *drv, Error **errp)
+{
+    return bdrv_open_inherit(pbs, filename, reference, options, flags, NULL,
+                             NULL, drv, errp);
+}
+
 typedef struct BlockReopenQueueEntry {
      bool prepared;
      BDRVReopenState state;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 1e92607..bc247f4 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -429,7 +429,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
     /* Open the backing file */
     assert(bs->file == NULL);
     ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, 
"image",
-                          flags | BDRV_O_PROTOCOL, false, &local_err);
+                          bs, &child_file, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/block/blkverify.c b/block/blkverify.c
index 438dff8..d277e63 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -125,7 +125,7 @@ static int blkverify_open(BlockDriverState *bs, QDict 
*options, int flags,
     /* Open the raw file */
     assert(bs->file == NULL);
     ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
-                          "raw", flags | BDRV_O_PROTOCOL, false, &local_err);
+                          "raw", bs, &child_file, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
@@ -134,7 +134,7 @@ static int blkverify_open(BlockDriverState *bs, QDict 
*options, int flags,
     /* Open the test file */
     assert(s->test_file == NULL);
     ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), 
options,
-                          "test", flags, false, &local_err);
+                          "test", bs, &child_format, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         s->test_file = NULL;
diff --git a/block/quorum.c b/block/quorum.c
index a33881a..77e55b2 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -935,8 +935,8 @@ static int quorum_open(BlockDriverState *bs, QDict 
*options, int flags,
         ret = snprintf(indexstr, 32, "children.%d", i);
         assert(ret < 32);
 
-        ret = bdrv_open_image(&s->bs[i], NULL, options, indexstr, flags,
-                              false, &local_err);
+        ret = bdrv_open_image(&s->bs[i], NULL, options, indexstr, bs,
+                              &child_format, false, &local_err);
         if (ret < 0) {
             goto close_exit;
         }
diff --git a/block/vmdk.c b/block/vmdk.c
index aad051b..3284bec 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -852,9 +852,8 @@ static int vmdk_parse_extents(const char *desc, 
BlockDriverState *bs,
         ret = snprintf(extent_opt_prefix, 32, "extents.%d", s->num_extents);
         assert(ret < 32);
 
-        ret = bdrv_open_image(&extent_file, extent_path,
-                              options, extent_opt_prefix,
-                              bs->open_flags | BDRV_O_PROTOCOL, false, errp);
+        ret = bdrv_open_image(&extent_file, extent_path, options,
+                              extent_opt_prefix, bs, &child_file, false, errp);
         g_free(extent_path);
         if (ret) {
             return ret;
diff --git a/include/block/block.h b/include/block/block.h
index 5f3c2de..45e2340 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -12,6 +12,7 @@
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 typedef struct BlockJob BlockJob;
+typedef struct BdrvChildRole BdrvChildRole;
 
 typedef struct BlockDriverInfo {
     /* in bytes, 0 if irrelevant */
@@ -203,7 +204,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState 
*bs_top);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_open_image(BlockDriverState **pbs, const char *filename,
-                    QDict *options, const char *bdref_key, int flags,
+                    QDict *options, const char *bdref_key,
+                    BlockDriverState* parent, const BdrvChildRole *child_role,
                     bool allow_none, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f004378..662dd56 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -330,6 +330,13 @@ typedef struct BdrvAioNotifier {
     QLIST_ENTRY(BdrvAioNotifier) list;
 } BdrvAioNotifier;
 
+struct BdrvChildRole {
+    int (*inherit_flags)(int parent_flags);
+};
+
+extern const BdrvChildRole child_file;
+extern const BdrvChildRole child_format;
+
 /*
  * Note: the function bdrv_append() copies and swaps contents of
  * BlockDriverStates, so if you add new fields to this struct, please
-- 
1.8.3.1