Re: [Qemu-block] [PATCH 07/34] block: Move flag inheritance to bdrv_open_inherited()

[Kevin Wolf]

Am 11.05.2015 um 17:20 hat Max Reitz geschrieben:
> On 08.05.2015 19:21, Kevin Wolf wrote:
> >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>
> >---
> >  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 cea022f..c4f0fb4 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,
> 
> Stern zur Variable!

Okay, now that we're in nitpicking mode:

Zur Variablen, wenn schon.

(Sorry for starting a discussion about German grammar on qemu-devel, but
I just have to...)

> >+                             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;
> >@@ -672,8 +678,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)
> >  {
> >@@ -690,6 +696,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
> >@@ -705,6 +730,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;
> >@@ -827,7 +856,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;
> >@@ -1134,9 +1162,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;
> >@@ -1170,7 +1199,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;
> >@@ -1198,7 +1228,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);
> >@@ -1285,9 +1316,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,
> 
> Faithfully following the forward declaration, so the same issue here.
> 
> Other than these nitpicks:
> 
> Reviewed-by: Max Reitz <address@hidden>

Thanks, will fix.

Kevin