Re: [Qemu-devel] [RFC PATCH 15/41] block: Add permissions to blk_new()

[Max Reitz]

On 13.02.2017 18:22, Kevin Wolf wrote:
> We want every user to be specific about the permissions it needs, so
> we'll pass the initial permissions as parameters to blk_new(). A user
> only needs to call blk_set_perm() if it wants to change the permissions
> after the fact.
> 
> The permissions are stored in the BlockBackend and applied whenever a
> BlockDriverState should be attached in blk_insert_bs().
> 
> This does not include actually choosing the right set of permissions
> yet. Instead, the usual FIXME comment is added to each place and will be
> addressed in individual patches.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/backup.c                   |  3 ++-
>  block/block-backend.c            | 12 ++++++------
>  block/commit.c                   | 12 ++++++++----
>  block/mirror.c                   |  3 ++-
>  blockdev.c                       |  2 +-
>  blockjob.c                       |  3 ++-
>  hmp.c                            |  3 ++-
>  hw/block/fdc.c                   |  3 ++-
>  hw/core/qdev-properties-system.c |  3 ++-
>  hw/ide/qdev.c                    |  3 ++-
>  hw/scsi/scsi-disk.c              |  3 ++-
>  include/sysemu/block-backend.h   |  2 +-
>  migration/block.c                |  3 ++-
>  nbd/server.c                     |  3 ++-
>  tests/test-blockjob.c            |  3 ++-
>  tests/test-throttle.c            |  7 ++++---
>  16 files changed, 42 insertions(+), 26 deletions(-)

[...]

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 8f0348d..a219f0b 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -123,14 +123,14 @@ static const BdrvChildRole child_root = {
>   * Store an error through @errp on failure, unless it's null.
>   * Return the new BlockBackend on success, null on failure.
>   */
> -BlockBackend *blk_new(void)
> +BlockBackend *blk_new(uint64_t perm, uint64_t shared_perm)

I think it would be a good idea to document what these parameters do, at
least by pointing to the definition of the permission flags.

Also, this makes me think that the permission flags should not be in
block_int.h but in block.h. This function is available outside of the
core block layer.

>  {
>      BlockBackend *blk;
>  
>      blk = g_new0(BlockBackend, 1);
>      blk->refcnt = 1;
> -    blk->perm = 0;
> -    blk->shared_perm = BLK_PERM_ALL;
> +    blk->perm = perm;
> +    blk->shared_perm = shared_perm;
>      blk_set_enable_write_cache(blk, true);
>  
>      qemu_co_queue_init(&blk->public.throttled_reqs[0]);
> @@ -161,7 +161,7 @@ BlockBackend *blk_new_open(const char *filename, const 
> char *reference,
>      BlockBackend *blk;
>      BlockDriverState *bs;
>  
> -    blk = blk_new();
> +    blk = blk_new(0, BLK_PERM_ALL);
>      bs = bdrv_open(filename, reference, options, flags, errp);
>      if (!bs) {
>          blk_unref(blk);
> @@ -505,9 +505,9 @@ void blk_remove_bs(BlockBackend *blk)
>  void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
>  {
>      bdrv_ref(bs);
> -    /* FIXME Use real permissions */
>      blk->root = bdrv_root_attach_child(bs, "root", &child_root,
> -                                       0, BLK_PERM_ALL, blk, &error_abort);
> +                                       blk->perm, blk->shared_perm, blk,
> +                                       &error_abort);

And the error_abort does not qualify as a FIXME?

>  
>      notifier_list_notify(&blk->insert_bs_notifiers, blk);
>      if (blk->public.throttle_state) {

[...]

> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 068c9e4..1dd1cfa 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -53,7 +53,8 @@ static BlockJob *do_test_id(BlockBackend *blk, const char 
> *id,
>   * BlockDriverState inserted. */
>  static BlockBackend *create_blk(const char *name)
>  {
> -    BlockBackend *blk = blk_new();
> +    /* FIXME Use real permissions */
> +    BlockBackend *blk = blk_new(0, BLK_PERM_ALL);
>      BlockDriverState *bs;
>  
>      bs = bdrv_open("null-co://", NULL, NULL, 0, &error_abort);

Pretty much independent of this patch, but: blk_new_open() would
probably be the better choice then to call blk_new() + bdrv_open() +
blk_insert_bs().

Max

signature.asc
Description: OpenPGP digital signature