From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44863) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YrpW1-0002eZ-CA for qemu-devel@nongnu.org; Mon, 11 May 2015 11:21:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YrpVu-0000xq-AA for qemu-devel@nongnu.org; Mon, 11 May 2015 11:21:09 -0400 Message-ID: <5550C8D8.3080505@redhat.com> Date: Mon, 11 May 2015 17:20:56 +0200 From: Max Reitz MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-8-git-send-email-kwolf@redhat.com> In-Reply-To: <1431105726-3682-8-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit 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: Kevin Wolf , qemu-block@nongnu.org Cc: armbru@redhat.com, qemu-devel@nongnu.org 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! > + 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