Re: [Qemu-block] [PATCH v7 03/20] block: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

Am 08.08.2016 um 15:13 hat Fam Zheng geschrieben:
> Respect the locking mode from CLI or QMP, and set the open flags
> accordingly.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c               | 21 +++++++++++++++++++++
>  blockdev.c            |  9 +++++++++
>  include/block/block.h |  1 +
>  qemu-options.hx       |  1 +
>  4 files changed, 32 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 30d64e6..5f76f60 100644
> --- a/block.c
> +++ b/block.c
> @@ -705,6 +705,7 @@ static void bdrv_inherited_options(int *child_flags, 
> QDict *child_options,
>       * the parent. */
>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
>      qdict_copy_default(child_options, parent_options, 
> BDRV_OPT_CACHE_NO_FLUSH);
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_LOCK_MODE);
>  
>      /* Our block drivers take care to send flushes and respect unmap policy,
>       * so we can default to enable both on lower layers regardless of the
> @@ -757,6 +758,7 @@ static void bdrv_backing_options(int *child_flags, QDict 
> *child_options,
>       * which is only applied on the top level (BlockBackend) */
>      qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
>      qdict_copy_default(child_options, parent_options, 
> BDRV_OPT_CACHE_NO_FLUSH);
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_LOCK_MODE);
>  
>      /* backing files always opened read-only */
>      flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ);
> @@ -880,6 +882,11 @@ static QemuOptsList bdrv_runtime_opts = {
>              .name = BDRV_OPT_CACHE_NO_FLUSH,
>              .type = QEMU_OPT_BOOL,
>              .help = "Ignore flush requests",
> +        },{
> +            .name = BDRV_OPT_LOCK_MODE,
> +            .type = QEMU_OPT_STRING,
> +            .help = "how to lock the image (auto, shared, off. "
> +                    "default: auto)",
>          },
>          { /* end of list */ }
>      },
> @@ -897,6 +904,7 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BdrvChild *file,
>      const char *filename;
>      const char *driver_name = NULL;
>      const char *node_name = NULL;
> +    const char *lock_mode = NULL;
>      QemuOpts *opts;
>      BlockDriver *drv;
>      Error *local_err = NULL;
> @@ -940,6 +948,19 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BdrvChild *file,
>          goto fail_opts;
>      }
>  
> +    lock_mode = qemu_opt_get(opts, BDRV_OPT_LOCK_MODE) ? : "off";

Am I missing some other place that overrides this or does it make the
default "off" rather than "auto"?

> +    if (!strcmp(lock_mode, "auto")) {
> +        /* Default */
> +    } else if (!strcmp(lock_mode, "shared")) {
> +        bs->open_flags |= BDRV_O_SHARED_LOCK;
> +    } else if (!strcmp(lock_mode, "off")) {
> +        bs->open_flags |= BDRV_O_NO_LOCK;
> +    } else {
> +        error_setg(errp, "invalid lock mode");
> +        ret = -EINVAL;
> +        goto fail_opts;
> +    }
> +
>      bs->read_only = !(bs->open_flags & BDRV_O_RDWR);

The other thing is that we didn't really finish the discussion whether
leaving the configuration on the node level to the user is a good idea
when the restrictions that the user can influence rather come from the
top level (the guest OS) and only propagate down the BDS tree.

Aren't we requiring that the user understands the internal workings of
qemu in order to infer which nodes need to be locked in which mode,
rather than just saying "my guest is okay with sharing this disk, you
can figure out what this means for locking"?

Kevin