[PATCH 1/2] qemu: introduce locking option for disk source of qemu

[Peter Krempa]

On Fri, Jan 22, 2021 at 21:32:05 -0500, Masayoshi Mizuma wrote:
> From: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> 
> Introduce locking option for disk source of qemu.
> It may be useful to avoid file lock issues.

Lock issues usually mean that you are doing something wrong.

> locking option supports three switches; 'auto', 'on' and 'off'.
> The default behaivor will work if locking option isn't set.
> 
> Example of the usage:
> 
>   <disk type='file' device='disk'>
>     <driver name='qemu' type='qcow2' cache='none'/>
>     <source file='/tmp/QEMUGuest1.img' locking='off'/>
>     <target dev='hda' bus='ide'/>
>     <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>   </disk>
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
> ---
>  docs/schemas/domaincommon.rng  |  9 +++++++++
>  src/conf/domain_conf.c         |  8 ++++++++
>  src/conf/storage_source_conf.c |  9 +++++++++
>  src/conf/storage_source_conf.h | 14 ++++++++++++++
>  src/libvirt_private.syms       |  1 +
>  src/qemu/qemu_block.c          |  5 +++++
>  6 files changed, 46 insertions(+)

Few quick notes:

- I'm not entirely persuaded that we should at this at all
    - qemu locking works to prevent user mistakes
    - you really need to know what to expect if you use images which are
      used by multiple VMs

    - you really need to add some form of justification, the current one
      definitely is insufficient and thus I'm inclined to NACKing the
      concept of enabling setting of the locks completely

      Please describe the use case in details.

- documentation is completely missing
    - note that it should be worded such that it sternly discourages
      touching this option at all

- the patch must be split at least into two
    - adding the XML bits and docs
    - qemu bits

- it's only implemented for 'VIR_STORAGE_TYPE_BLOCK' and
  'VIR_STORAGE_TYPE_FILE' but the patch is completely missing a check
  that it's used with a network or other disk which doesn't actually
  support locking.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index dab4f10326..067ffa877b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8540,6 +8540,7 @@ virDomainStorageSourceParse(xmlNodePtr node,
>  {
>      VIR_XPATH_NODE_AUTORESTORE(ctxt)
>      xmlNodePtr tmp;
> +    char *locking;
>  
>      ctxt->node = node;
>  
> @@ -8606,6 +8607,9 @@ virDomainStorageSourceParse(xmlNodePtr node,
>              return -1;
>      }
>  
> +    if ((locking = virXMLPropString(node, "locking")))
> +        src->locking = virStorageFileLockingTypeFromString(locking);

'locking' temporary variable is leaked

> +
>      return 0;
>  }
>  


> diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
> index e66ccdedef..6f5b165504 100644
> --- a/src/conf/storage_source_conf.h
> +++ b/src/conf/storage_source_conf.h

[...]

> @@ -394,6 +406,8 @@ struct _virStorageSource {
>      char *nfs_group;
>      uid_t nfs_uid;
>      gid_t nfs_gid;
> +
> +    int locking; /* enum virStorageFileLocking */

    We prefer to use the enum type directly if possible. It requires a
    temproary variable in the parser though.
>  };
>  
>  G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageSource, virObjectUnref);

[...]

> @@ -1023,12 +1024,16 @@ qemuBlockStorageSourceGetFileProps(virStorageSourcePtr src,
>  
>          if (src->iomode != VIR_DOMAIN_DISK_IO_DEFAULT)
>              iomode = virDomainDiskIoTypeToString(src->iomode);
> +
> +        if (src->locking != VIR_STORAGE_FILE_LOCKING_DEFAULT)
> +            locking = virStorageFileLockingTypeToString(src->locking);
>      }
>  
>      ignore_value(virJSONValueObjectCreate(&ret,
>                                            "s:filename", src->path,
>                                            "S:aio", iomode,
>                                            "S:pr-manager", prManagerAlias,
> +                                          "S:locking", locking,
>                                            NULL) < 0);
>      return ret;
>  }
> -- 
> 2.27.0
>