Re: [libvirt] [PATCH v4 09/14] qemu: Introduce pr_helper to qemu.conf

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> Just like we allow users overriding path to bridge-helper
> detected at compile time we can allow them to override path to
> qemu-pr-helper.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  m4/virt-driver-qemu.m4 | 5 +
>  src/qemu/libvirtd_qemu.aug | 1 +
>  src/qemu/qemu.conf | 4 
>  src/qemu/qemu_conf.c   | 7 ++-
>  src/qemu/qemu_conf.h   | 1 +
>  src/qemu/test_libvirtd_qemu.aug.in | 1 +
>  6 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4
> index b9bafdab90..80e1d3ad46 100644
> --- a/m4/virt-driver-qemu.m4
> +++ b/m4/virt-driver-qemu.m4
> @@ -57,6 +57,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [
> [/usr/libexec:/usr/lib/qemu:/usr/lib])
>AC_DEFINE_UNQUOTED([QEMU_BRIDGE_HELPER], ["$QEMU_BRIDGE_HELPER"],
>   [QEMU bridge helper])
> +  AC_PATH_PROG([QEMU_PR_HELPER], [qemu-pr-helper],
> +   [/usr/bin/qemu-pr-helper],
> +   [/usr/bin:/usr/libexec])

So the default install location of qemu-pr-helper is /usr/bin unlike
bridge-helper which is /usr/libexec?

> +  AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"],
> + [QEMU PR helper])
>  ])
>  
>  AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index c19bf3a43a..2dc16e91fd 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -86,6 +86,7 @@ module Libvirtd_qemu =
> let process_entry = str_entry "hugetlbfs_mount"
>   | bool_entry "clear_emulator_capabilities"
>   | str_entry "bridge_helper"
> + | str_entry "pr_helper"
>   | bool_entry "set_process_name"
>   | int_entry "max_processes"
>   | int_entry "max_files"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index 07eab7efff..30fdd54e2c 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -775,3 +775,7 @@
>  # This directory is used for memoryBacking source if configured as file.
>  # NOTE: big files will be stored here
>  #memory_backing_dir = "/var/lib/libvirt/qemu/ram"
> +
> +# Path to the SCSI persistent reservations helper. This helper is
> +# used whenever  are enabled for SCSI disks.

s/disks/LUN devices/

> +#pr_helper = "/usr/libexec/qemu-pr-helper"

Going with my note above - the default path in the m4 file shows
/usr/bin first... So should this match that? Similar to how
qemu-bridge-helper matches the /usr/libexec install location?

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 36cf3a281c..8c69dbe75c 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -307,7 +307,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool 
> privileged)
>  goto error;
>  }
>  
> -if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0)
> +if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 ||
> +VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0)
>  goto error;
>  
>  cfg->clearEmulatorCapabilities = true;
> @@ -392,6 +393,7 @@ static void virQEMUDriverConfigDispose(void *obj)
>  }
>  VIR_FREE(cfg->hugetlbfs);
>  VIR_FREE(cfg->bridgeHelperName);
> +VIR_FREE(cfg->prHelperName);
>  
>  VIR_FREE(cfg->saveImageFormat);
>  VIR_FREE(cfg->dumpImageFormat);
> @@ -759,6 +761,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr 
> cfg,
>  if (virConfGetValueString(conf, "bridge_helper", >bridgeHelperName) 
> < 0)
>  goto cleanup;
>  
> +if (virConfGetValueString(conf, "pr_helper", >prHelperName) < 0)
> +goto cleanup;
> +
>  if (virConfGetValueBool(conf, "mac_filter", >macFilter) < 0)
>  goto cleanup;
>  
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index e1ad5463f3..7a63780c48 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -153,6 +153,7 @@ struct _virQEMUDriverConfig {
>  size_t nhugetlbfs;
>  
>  char *bridgeHelperName;
> +char *prHelperName;
>  
>  bool macFilter;
>  
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in 
> b/src/qemu/test_libvirtd_qemu.aug.in
> index 688e5b9fda..c0efae47bd 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -100,3 +100,4 @@ module Test_libvirtd_qemu =
>  { "1" = "mount" }
>  }
>  { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" }
> +{ "pr_helper" = "/usr/libexec/qemu-pr-helper" }
> 

And don't forget this one if something does change, but I'm sure that'd
wash out of the check syntax-check

I think the questions/issues are minimal and easily fixed by a response,
so consider the concept at least

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com

Re: [libvirt] [PATCH v4 13/14] qemu_hotplug: Hotunplug of reservations

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> If we are the last one to use pr-manager object we need to remove
> it and also kill the qemu-pr-helper process.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_hotplug.c | 38 ++
>  1 file changed, 38 insertions(+)
> 

Again, my opinion this is combined too - patch is larger, but everything
is covered at one time.

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 43bb910ed6..98e1bf7082 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3894,6 +3894,34 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr 
> def,
>  }
>  
>  
> +static qemuDomainDiskPRDPtr
> +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm,
> +   virDomainDiskDefPtr disk)
> +{
> +qemuDomainStorageSourcePrivatePtr srcPriv;
> +size_t i;
> +
> +if (!virStoragePRDefIsManaged(disk->src->pr))
> +return NULL;
> +
> +for (i = 0; i < vm->def->ndisks; i++) {
> +const virDomainDiskDef *domainDisk = vm->def->disks[i];
> +
> +if (domainDisk == disk)
> +continue;
> +
> +if (virStoragePRDefIsManaged(domainDisk->src->pr))
> +break;
> +}
> +
> +if (i != vm->def->ndisks)
> +return NULL;
> +
> +srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +return srcPriv->prd;
> +}
> +
> +

Trying to combine two separate things into one?

Thing 1 -> Remove the object from the domain
Thing 2 -> Kill the PRD helper *iff* this is the last one using it

>  static int
>  qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> @@ -3907,6 +3935,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  char *drivestr;
>  char *objAlias = NULL;
>  char *encAlias = NULL;
> +qemuDomainDiskPRDPtr prd = NULL;
>  
>  VIR_DEBUG("Removing disk %s from domain %p %s",
>disk->info.alias, vm, vm->def->name);
> @@ -3944,6 +3973,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  }
>  }
>  
> +prd = qemuDomainDiskNeedRemovePR(vm, disk);
> +
>  qemuDomainObjEnterMonitor(driver, vm);
>  
>  qemuMonitorDriveDel(priv->mon, drivestr);
> @@ -3959,6 +3990,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  ignore_value(qemuMonitorDelObject(priv->mon, encAlias));
>  VIR_FREE(encAlias);
>  
> +/* If it fails, then so be it - it was a best shot */
> +if (prd)
> +ignore_value(qemuMonitorDelObject(priv->mon, prd->alias));
> +

OK - remove the object if we have one...

>  if (disk->src->haveTLS)
>  ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));
>  
> @@ -3977,6 +4012,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>  }
>  }
>  
> +if (prd)
> +qemuProcessKillPRDaemon(vm);
> +

But we only want to kill if this if there are no other disk's needing
the pr-helper, right?  So we need to have a routine that would be run
after the disk is really removed.  The two phase removal process always
gets me ;-)  - that is Detach vs. Remove and which one is last *after*
the disk is removed from the list of disks *and* when we know the object
has been removed from the domain, then we can stop the PR process iff
this is the last one using it.

John

>  qemuDomainReleaseDeviceAddress(vm, >info, src);
>  
>  if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 12/14] qemu_hotplug: Hotplug of reservations

[John Ferlan]

[...]

>   *
> @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>  bool driveAdded = false;
>  bool secobjAdded = false;
>  bool encobjAdded = false;
> +bool prmgrAdded = false;
>  bool prdStarted = false;
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  virJSONValuePtr secobjProps = NULL;
>  virJSONValuePtr encobjProps = NULL;
> +virJSONValuePtr prmgrProps = NULL;
>  qemuDomainStorageSourcePrivatePtr srcPriv;
>  qemuDomainSecretInfoPtr secinfo = NULL;
>  qemuDomainSecretInfoPtr encinfo = NULL;
> +qemuDomainDiskPRDPtr prd = NULL;
>  
>  if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0)
>  goto cleanup;
> @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>  if (srcPriv) {
>  secinfo = srcPriv->secinfo;
>  encinfo = srcPriv->encinfo;
> +prd = srcPriv->prd;
>  }
>  
>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>  if (encinfo && qemuBuildSecretInfoProps(encinfo, ) < 0)
>  goto error;
>  
> +if (qemuMaybeBuildPRManagerInfoProps(vm, prd, ) < 0)
> +goto error;
> +
>  if (disk->src->haveTLS &&
>  qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src,
>disk->info.alias) < 0)
> @@ -484,6 +514,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>  encobjAdded = true;
>  }
>  
> +if (prmgrProps) {
> +rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", prd->alias,
> +  prmgrProps);
> +prmgrProps = NULL; /* qemuMonitorAddObject consumes */
> +if (rv < 0)
> +goto exit_monitor;
> +prmgrAdded = true;
> +}
> +

Oh yeah - coverity let me know that @prmgrProps could be leaked if we
don't get this far - need to add the virJSONValueFree cleanup

>  if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>  goto exit_monitor;
>  driveAdded = true;

[...]

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list