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

[Peter Krempa]

On Mon, Feb 12, 2018 at 17:10:23 +, Daniel Berrange wrote:
> On Mon, Feb 12, 2018 at 05:54:19PM +0100, Peter Krempa wrote:
> > 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 
> > > ---
> > >  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, , ) < 0)
> > 
> > If this detects '0' namespaces ...
> > 
> > > +return ret;
> > > +
> > > +if (virProcessSetNamespaces(nfds, fds) < 0)
> > 
> > ... this call will be unhappy.
> 
> Namespaces have been around since at least RHEL-5 vintage so I reckon
> we can assume non-zero number of namespaces. In fact perhaps we should
> just make virProcessGetNamespaces() return an explicit error in the
> unlikely case it finds zero namespaces.

Fair enough, but it still does not look like the right usage semantics
in this case.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Daniel P . Berrangé]

On Mon, Feb 12, 2018 at 05:54:19PM +0100, Peter Krempa wrote:
> 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 
> > ---
> >  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, , ) < 0)
> 
> If this detects '0' namespaces ...
> 
> > +return ret;
> > +
> > +if (virProcessSetNamespaces(nfds, fds) < 0)
> 
> ... this call will be unhappy.

Namespaces have been around since at least RHEL-5 vintage so I reckon
we can assume non-zero number of namespaces. In fact perhaps we should
just make virProcessGetNamespaces() return an explicit error in the
unlikely case it finds zero namespaces.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [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 
> ---
>  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, , ) < 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 = 50; /* 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, ) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("pr helper %s didn't show up"), (const char *) 
> name);
> +goto cleanup;
> +}
> +
> +if (virTimeBackOffStart(, 1, timeout) < 0)
> +goto cleanup;
> +while (virTimeBackOffWait()) {
> +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())
> +   

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

[Michal Privoznik]

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 
---
 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, , ) < 0)
+return ret;
+
+if (virProcessSetNamespaces(nfds, fds) < 0)
+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 = 50; /* ms */
+int ret = -1;
+
+if (!prObj->managed)
+return 0;
+
+if (!virFileIsExecutable(cfg->prHelperName)) {
+virReportSystemError(errno, _("'%s' is not a suitable pr helper"),
+ 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;
+#endif
+
+if (virCommandRun(cmd, NULL) < 0)
+goto cleanup;
+
+if (virPidFileReadPath(pidfile, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("pr helper %s didn't show up"), (const char *) name);
+goto cleanup;
+}
+
+if (virTimeBackOffStart(, 1, timeout) < 0)
+goto cleanup;
+while (virTimeBackOffWait()) {
+if (virFileExists(prObj->path))
+break;
+
+if (virProcessKill(cpid, 0) == 0)
+continue;
+
+virReportSystemError(errno, "%s",
+ _("pr helper died unexpectedly"));
+goto cleanup;
+}
+
+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;
+}
+
+
+static int
+qemuProcessSetupPRDaemons(virDomainObjPtr vm)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+int ret = -1;
+
+if (priv->prHelpers &&
+virHashForEach(priv->prHelpers,
+   qemuProcessSetupOnePRDaemon, vm) < 0)
+goto cleanup;
+
+ret = 0;
+ cleanup:
+return ret;
+}
+
+
 static int
 qemuProcessInitPasswords(virConnectPtr conn,
  virQEMUDriverPtr driver,
@@ -5896,6 +6041,10 @@ qemuProcessLaunch(virConnectPtr conn,
 if (qemuProcessSetupEmulator(vm) < 0)
 goto cleanup;
 
+VIR_DEBUG("Setting up PR daemons");