[libvirt] [PATCH v3 10/12] qemu_hotplug: Hotplug of reservations

[Peter Krempa]

On Wed, Mar 14, 2018 at 17:05:39 +0100, Michal Privoznik wrote:
> Surprisingly, nothing special is happening here. If we are the
> first to use the managed helper then spawn it. If not, we're
> almost done.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_process.c | 38 +++++++++++++++++++++-----
>  src/qemu/qemu_process.h |  7 +++++
>  3 files changed, 110 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e0a5300f0..8cc0b631d 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -348,6 +348,58 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuBuildPRDefInfoProps(virDomainObjPtr vm,
> +                        virDomainDiskDefPtr disk,
> +                        virJSONValuePtr *prmgrProps,
> +                        const char **prAlias,
> +                        const char **prPath)

It looks like this function is doing a bit too much for the name. It's
not only creating the JSON props of the object but setting up the
daemon. That looks like should be separated.

> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    qemuDomainStorageSourcePrivatePtr srcPriv;
> +    virJSONValuePtr props = NULL;
> +    int ret = -1;
> +
> +    srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +
> +    *prmgrProps = NULL;
> +
> +    if (priv->prPid != (pid_t) -1 ||
> +        !srcPriv->prd ||
> +        !srcPriv->prd->alias)
> +        return 0;

So, this will exit early if you have the managed daemon, so you will not
add the object for the unmanaged option if hotplugging?

As said above, this function does too much. It should be split and then
you will not have these problems.

> +    if (virJSONValueObjectCreate(&props,
> +                                 "s:path", srcPriv->prd->path,
> +                                 NULL) < 0)
> +        goto cleanup;
> +
> +    if (qemuProcessSetupOnePRDaemon(vm, disk) < 0)
> +        goto cleanup;
> +
> +    *prAlias = srcPriv->prd->alias;
> +    *prPath = srcPriv->prd->path;
> +    *prmgrProps = props;
> +    props = NULL;
> +    ret = 0;
> + cleanup:
> +    virJSONValueFree(props);
> +    return ret;
> +}
> +
> +
> +static void
> +qemuDestroyPRDefObject(virDomainObjPtr vm,
> +                       const char *alias,
> +                       const char *path)
> +{
> +    if (!alias)
> +        return;
> +
> +    qemuProcessKillPRDaemon(vm, path, false);

This does not make much sense. You only ever need to kill the managed
one, so passing random path is weird. This should be called only for the
managed daemon.

> +}
> +
> +
>  /**
>   * qemuDomainAttachDiskGeneric:
>   *

[...]

> @@ -403,6 +459,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>                                        disk->info.alias) < 0)
>          goto error;
>  
> +    if (qemuBuildPRDefInfoProps(vm, disk, &prmgrProps, &prAlias, &prPath) < 0)
> +        goto error;
> +
>      if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps)))
>          goto error;
>  
> @@ -435,6 +494,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>          encobjAdded = true;
>      }
>  
> +    if (prmgrProps) {
> +        rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", prAlias,
> +                                  prmgrProps);
> +        prmgrProps = NULL; /* qemuMonitorAddObject consumes */
> +        if (rv < 0)
> +            goto exit_monitor;
> +        prmgrAdded = true;
> +    }
> +
>      if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>          goto exit_monitor;
>      driveAdded = true;

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index b876d293a..cb160727c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2556,16 +2556,40 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver,
>  }
>  
>  
> -static void
> -qemuProcessKillPRDaemon(virDomainObjPtr vm)
> +void
> +qemuProcessKillPRDaemon(virDomainObjPtr vm,
> +                        const char *socketPath,

socketPath seems useless here ...

> +                        bool force)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    size_t nmanaged = 0;
> +    size_t i;
>  
>      if (priv->prPid == (pid_t) -1)
>          return;
>  
> -    virProcessKillPainfully(priv->prPid, true);
> -    priv->prPid = (pid_t) -1;
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        qemuDomainStorageSourcePrivatePtr srcPriv;
> +        virDomainDiskDefPtr disk = vm->def->disks[i];
> +
> +        if (!virStoragePRDefIsManaged(disk->src->pr))
> +            continue;
> +
> +        nmanaged++;
> +
> +        srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src);
> +        if (!socketPath)
> +            socketPath = srcPriv->prd->path;
> +    }
> +
> +    if (force || nmanaged <= 1) {
> +        virProcessKillPainfully(priv->prPid, true);

since this will be true only for the managed daemons, thus you'll have
the socket path from the definition.

> +        priv->prPid = (pid_t) -1;
> +        if (socketPath &&
> +            unlink(socketPath) < 0 &&
> +            errno != ENOENT)
> +            VIR_WARN("Unable to remove pr helper socket %s", socketPath);
> +    }
>  }
>  
>  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180315/133a77ba/attachment-0001.sig>