[libvirt] [PATCH 1/8] util: Replace virDomainDiskSourceIsBlockType with a new helper

[John Ferlan]

On 05/02/2016 10:32 AM, Peter Krempa wrote:
> For disks sources described by a libvirt volume we don't need to do a
> complicated check since virStorageTranslateDiskSourcePool already
> correctly determines the actual disk type.
> 
> Replace the checks using a new accessor that does not open-code the
> whole logic.
> ---
>  src/libvirt_private.syms  |  1 +
>  src/lxc/lxc_cgroup.c      |  3 ++-
>  src/qemu/qemu_conf.c      | 10 +++++++---
>  src/util/virstoragefile.c | 16 +++++++++++++++-
>  src/util/virstoragefile.h |  3 ++-
>  5 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5030ec3..73c9fdb 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2277,6 +2277,7 @@ virStorageSourceFree;
>  virStorageSourceGetActualType;
>  virStorageSourceGetSecurityLabelDef;
>  virStorageSourceInitChainElement;
> +virStorageSourceIsBlockLocal;
>  virStorageSourceIsEmpty;
>  virStorageSourceIsLocalStorage;
>  virStorageSourceNewFromBacking;
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 4afe7e2..ea86d36 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -393,7 +393,8 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
> 
>      VIR_DEBUG("Allowing any disk block devs");
>      for (i = 0; i < def->ndisks; i++) {
> -        if (!virDomainDiskSourceIsBlockType(def->disks[i]->src, false))
> +        if (virStorageSourceIsEmpty(def->disks[i]->src) ||
> +            !virStorageSourceIsBlockLocal(def->disks[i]->src))

Could an LXC domain use a storage pool for a volume? If so, then
somewhere in the LXC code wouldn't we need to translate the source pool
in order to get "actualtype" and the volume check...


>              continue;
> 
>          if (virCgroupAllowDevicePath(cgroup,
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 5ed5776..e00ddca 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1243,7 +1243,9 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
>      char *key = NULL;
>      int ret = -1;
> 
> -    if (!disk->src->shared || !virDomainDiskSourceIsBlockType(disk->src, false))
> +    if (virStorageSourceIsEmpty(disk->src) ||
> +        !disk->src->shared ||
> +        !virStorageSourceIsBlockLocal(disk->src))
>          return 0;
> 
>      qemuDriverLock(driver);
> @@ -1388,7 +1390,9 @@ qemuRemoveSharedDisk(virQEMUDriverPtr driver,
>      char *key = NULL;
>      int ret = -1;
> 
> -    if (!disk->src->shared || !virDomainDiskSourceIsBlockType(disk->src, false))
> +    if (virStorageSourceIsEmpty(disk->src) ||
> +        !disk->src->shared ||
> +        !virStorageSourceIsBlockLocal(disk->src))
>          return 0;
> 
>      qemuDriverLock(driver);
> @@ -1476,7 +1480,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
>          disk = dev->data.disk;
> 
>          if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN ||
> -            !virDomainDiskSourceIsBlockType(disk->src, false))
> +            !virStorageSourceIsBlockLocal(disk->src))
>              return 0;
> 
>          path = virDomainDiskGetSource(disk);
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 05ac254..4f44e05 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1955,7 +1955,7 @@ virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def)
> 
> 
>  int
> -virStorageSourceGetActualType(virStorageSourcePtr def)
> +virStorageSourceGetActualType(const virStorageSource *def)
>  {
>      if (def->type == VIR_STORAGE_TYPE_VOLUME && def->srcpool)
>          return def->srcpool->actualtype;
> @@ -2012,6 +2012,20 @@ virStorageSourceIsEmpty(virStorageSourcePtr src)
> 
> 
>  /**
> + * virStorageSourceIsBlockLocal:
> + * @src: disk source definition
> + *
> + * Returns true if @src describes a locally accessible block storage source.
> + * This includes block devices and host-mapped iSCSI volumes.
> + */
> +bool
> +virStorageSourceIsBlockLocal(const virStorageSource *src)
> +{
> +    return virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK;

This assumes that virStorageTranslateDiskSourcePool has been run, which
is true is for the non-LXC paths changed in this patch...

So as long as you're comfortable with the LXC adjustment, then

ACK -

John

> +}
> +
> +
> +/**
>   * virStorageSourceBackingStoreClear:
>   *
>   * @src: disk source to clear
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index b98fe25..17e1277 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -357,9 +357,10 @@ int virStorageSourceInitChainElement(virStorageSourcePtr newelem,
>                                       bool force);
>  void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def);
>  void virStorageSourceClear(virStorageSourcePtr def);
> -int virStorageSourceGetActualType(virStorageSourcePtr def);
> +int virStorageSourceGetActualType(const virStorageSource *def);
>  bool virStorageSourceIsLocalStorage(virStorageSourcePtr src);
>  bool virStorageSourceIsEmpty(virStorageSourcePtr src);
> +bool virStorageSourceIsBlockLocal(const virStorageSource *src);
>  void virStorageSourceFree(virStorageSourcePtr def);
>  void virStorageSourceBackingStoreClear(virStorageSourcePtr def);
>  int virStorageSourceUpdateBlockPhysicalSize(virStorageSourcePtr src,
>