From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46743) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsAJ5-0005Ja-T5 for qemu-devel@nongnu.org; Tue, 12 May 2015 09:33:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsAIz-00062d-RP for qemu-devel@nongnu.org; Tue, 12 May 2015 09:33:11 -0400 Date: Tue, 12 May 2015 15:32:57 +0200 From: Kevin Wolf Message-ID: <20150512133257.GB3524@noname.str.redhat.com> References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-8-git-send-email-kwolf@redhat.com> <5550C8D8.3080505@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5550C8D8.3080505@redhat.com> Subject: Re: [Qemu-devel] [PATCH 07/34] block: Move flag inheritance to bdrv_open_inherited() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: armbru@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org 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 > >--- > > 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 Thanks, will fix. Kevin