[libvirt] [PATCH v4 11/14] qemu: Start PR daemon on disk hotplug

[John Ferlan]

On 04/10/2018 10:58 AM, Michal Privoznik wrote:
> When plugging a disk into domain we need to start qemu-pr-helper
> process iff this is the first disk with PR enabled for the
> domain. Otherwise the helper is already running (or not needed).
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_process.c |  4 ++--
>  src/qemu/qemu_process.h |  5 +++++
>  3 files changed, 58 insertions(+), 2 deletions(-)
> 

Personally not a fan of splitting up the hotplug need for PR daemon from
need for adding the object to the disk and the separate unplug patch,
but I understand why it's done this way.

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f0d549de38..468153c79c 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -348,6 +348,49 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>  }
>  
>  
> +/**
> + * qemuDomainMaybeStartPRDaemon:
> + * @vm: domain object
> + * @disk: disk to hotplug
> + *
> + * Checks if it's needed to start qemu-pr-helper and starts it.
> + *
> + * Returns: 0 if qemu-pr-helper is not needed
> + *          1 if it is needed and was started
> + *         -1 otherwise.
> + */
> +static int
> +qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm,
> +                             virDomainDiskDefPtr disk)
> +{
> +    size_t i;
> +    qemuDomainStorageSourcePrivatePtr srcPriv;
> +
> +    if (!virStoragePRDefIsManaged(disk->src->pr)) {
> +        /* @disk itself does not require qemu-pr-helper. */
> +        return 0;
> +    }
> +

If we used the fact that the thing was already started for the domain in
some sort of domain private data (which could even store the PID-file
path), then we wouldn't need the following...

> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        const virDomainDiskDef *domainDisk = vm->def->disks[i];
> +
> +        if (virStoragePRDefIsManaged(domainDisk->src->pr)) {
> +            /* qemu-pr-helper should be already started because
> +             * another disk in domain requires it. */
> +            return 0;
> +        }
> +    }
> +
> +    /* @disk requires qemu-pr-helper but none is running.
> +     * Start it now. */
> +    srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +    if (qemuProcessStartPRDaemon(vm, srcPriv->prd) < 0)
> +        return -1;

and the above would set things for us (conceivably)


John

> +
> +    return 1;
> +}
> +
> +
>  /**
>   * qemuDomainAttachDiskGeneric:
>   *
> @@ -368,6 +411,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>      bool driveAdded = false;
>      bool secobjAdded = false;
>      bool encobjAdded = false;
> +    bool prdStarted = false;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      virJSONValuePtr secobjProps = NULL;
>      virJSONValuePtr encobjProps = NULL;
> @@ -384,6 +428,11 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>      if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0)
>          goto error;
>  
> +    if ((rv = qemuDomainMaybeStartPRDaemon(vm, disk)) < 0)
> +        goto cleanup;
> +    else if (rv > 0)
> +        prdStarted = true;
> +
>      srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
>      if (srcPriv) {
>          secinfo = srcPriv->secinfo;
> @@ -481,6 +530,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>   error:
>      qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src);
>      ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true));
> +    if (prdStarted)
> +        qemuProcessKillPRDaemon(vm);
>      goto cleanup;
>  }
>  
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 982c989a0a..11aaeabb38 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2571,7 +2571,7 @@ qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm,
>  }
>  
>  
> -static void
> +void
>  qemuProcessKillPRDaemon(virDomainObjPtr vm)
>  {
>      virErrorPtr orig_err;
> @@ -2629,7 +2629,7 @@ qemuProcessStartPRDaemonHook(void *opaque)
>  }
>  
>  
> -static int
> +int
>  qemuProcessStartPRDaemon(virDomainObjPtr vm,
>                           qemuDomainDiskPRDPtr prd)
>  {
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 2741115673..8df5832e23 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -203,4 +203,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver,
>                              virDomainObjPtr vm,
>                              qemuDomainAsyncJob asyncJob);
>  
> +int qemuProcessStartPRDaemon(virDomainObjPtr vm,
> +                             qemuDomainDiskPRDPtr prd);
> +
> +void qemuProcessKillPRDaemon(virDomainObjPtr vm);
> +
>  #endif /* __QEMU_PROCESS_H__ */
>