[libvirt] [PATCH 09/14] qemu: Start PR daemons on domain startup

[Peter Krempa]

On Thu, Jan 18, 2018 at 17:04:41 +0100, Michal Privoznik wrote:
> Before we exec() qemu we have to spawn pr-helper processes for
> all managed reservations (well, technically there can only one).
> The only caveat there is that we should place the process into
> the same namespace and cgroup as qemu (so that it shares the same
> view of the system). But we can do that only after we've forked.
> That means calling the setup function between fork() and exec().
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_process.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 151 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 25ec464d3..02608c1f3 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2507,6 +2507,151 @@ qemuProcessSetupEmulator(virDomainObjPtr vm)
>  }
>  
>  
> +static int
> +qemuProcessSetupOnePRDaemonHook(void *opaque)
> +{
> +    virDomainObjPtr vm = opaque;
> +    size_t i, nfds = 0;
> +    int *fds = NULL;
> +    int ret = -1;
> +
> +    if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0)

If this detects '0' namespaces ...

> +        return ret;
> +
> +    if (virProcessSetNamespaces(nfds, fds) < 0)

... this call will be unhappy.

> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    for (i = 0; i < nfds; i++)
> +        VIR_FORCE_CLOSE(fds[i]);
> +    VIR_FREE(fds);
> +    return ret;
> +}
> +
> +
> +static int
> +qemuProcessSetupOnePRDaemon(void *payload,
> +                            const void *name,
> +                            void *opaque)
> +{
> +    qemuDomainDiskPRObjectPtr prObj = payload;
> +    virDomainObjPtr vm = opaque;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverPtr driver = priv->driver;
> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    char *pidfile = NULL;
> +    pid_t cpid = -1;
> +    virCommandPtr cmd = NULL;
> +    virTimeBackOffVar timebackoff;
> +    const unsigned long long timeout = 500000; /* ms */
> +    int ret = -1;
> +
> +    if (!prObj->managed)
> +        return 0;

Again. It does not make much sense to me to have the hash table and
everything if this is ever going to be called for the managed daemon.

> +
> +    if (!virFileIsExecutable(cfg->prHelperName)) {
> +        virReportSystemError(errno, _("'%s' is not a suitable pr helper"),

This error message does not really report what it's checking.

> +                             cfg->prHelperName);
> +        goto cleanup;
> +    }
> +
> +    if (!(pidfile = virPidFileBuildPath(cfg->stateDir, name)))
> +        goto cleanup;
> +
> +    if (unlink(pidfile) < 0 &&
> +        errno != ENOENT) {
> +        virReportSystemError(errno,
> +                             _("Cannot remove stale PID file %s"),
> +                             pidfile);
> +        goto cleanup;
> +    }
> +
> +    if (!(cmd = virCommandNewArgList(cfg->prHelperName,
> +                                     "-k", prObj->path,
> +                                     NULL)))
> +        goto cleanup;
> +
> +    virCommandDaemonize(cmd);
> +    virCommandSetPidFile(cmd, pidfile);
> +    virCommandSetPreExecHook(cmd, qemuProcessSetupOnePRDaemonHook, vm);
> +
> +#ifdef CAP_SYS_RAWIO
> +    virCommandAllowCap(cmd, CAP_SYS_RAWIO);
> +#else
> +    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                   _("Raw I/O is not supported on this platform"));
> +    goto cleanup;

This also looks like worth putting in the validation code for this so
that we don't really get to here to find out.


> +#endif
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    if (virPidFileReadPath(pidfile, &cpid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("pr helper %s didn't show up"), (const char *) name);
> +        goto cleanup;
> +    }
> +
> +    if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0)
> +        goto cleanup;
> +    while (virTimeBackOffWait(&timebackoff)) {
> +        if (virFileExists(prObj->path))
> +            break;
> +
> +        if (virProcessKill(cpid, 0) == 0)
> +            continue;
> +
> +        virReportSystemError(errno, "%s",
> +                             _("pr helper died unexpectedly"));
> +        goto cleanup;
> +    }

Sooo, after the timeout you assume success? That does not seem like a
reasonable idea.

> +
> +    if (priv->cgroup &&
> +        virCgroupAddMachineTask(priv->cgroup, cpid) < 0)
> +        goto cleanup;
> +
> +    if (qemuSecurityDomainSetPathLabel(driver->securityManager,
> +                                       vm->def, prObj->path, true) < 0)
> +        goto cleanup;
> +
> +    prObj->pid = cpid;
> +    ret = 0;
> + cleanup:
> +    if (ret < 0) {
> +        virCommandAbort(cmd);
> +        virProcessKillPainfully(cpid, true);
> +    }
> +    virCommandFree(cmd);
> +    if (unlink(pidfile) < 0 &&
> +        errno != ENOENT &&
> +        !virGetLastError())
> +        virReportSystemError(errno,
> +                             _("Cannot remove stale PID file %s"),
> +                             pidfile);
> +    VIR_FREE(pidfile);
> +    virObjectUnref(cfg);
> +    return ret;
> +}
-------------- 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/20180212/2a11c086/attachment-0001.sig>