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@xxxxxxxxxx>
> ---
> 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;
> +}
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
