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

[Max Reitz <mreitz@redhat.com>]

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 <kwolf@redhat.com>
> ---
>   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 <mreitz@redhat.com>