[libvirt PATCH 0/6] Some slirp/dbus related fixes
From: Marc-André Lureau <marcandre.lureau@redhat.com> Hi, Here is a subset of the series posted earlier "RFC: basic CGroup support with qemu:///session", as requested by Pavel. Marc-André Lureau (6): slirp: leave the dbus daemon running on error slirp: add helper to VM cgroup qemu-dbus: prevent double start of the bus daemon qemu-dbus: remove unused variable qemu-dbus: register DBus bus to the VM cgroup qemu-cgroup: remove unnecessary include src/qemu/qemu_cgroup.c | 1 - src/qemu/qemu_dbus.c | 29 +++++++++++++++++++++++++++-- src/qemu/qemu_dbus.h | 3 +++ src/qemu/qemu_extdevice.c | 12 ++++++++++++ src/qemu/qemu_slirp.c | 16 +++++++++++++++- src/qemu/qemu_slirp.h | 3 +++ 6 files changed, 60 insertions(+), 4 deletions(-) -- 2.26.0.106.g9fadedd637
From: Marc-André Lureau <marcandre.lureau@redhat.com> Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - qemuDBusStop(driver, vm); + /* leave dbus daemon running, it may be used by others */ return -1; } -- 2.26.0.106.g9fadedd637
On 4/8/20 7:23 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - qemuDBusStop(driver, vm); + /* leave dbus daemon running, it may be used by others */ return -1; }
I'm not quite sure about this one. Who do you mean by "others"? Other interfaces? Is this supposed to help with 3/6 so that if we attempt to double start the dbus daemon the second attempt doesn't actually kill the daemon started in the first attempt? ACK to the rest. Michal
Hi On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 4/8/20 7:23 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - qemuDBusStop(driver, vm); + /* leave dbus daemon running, it may be used by others */ return -1; }
I'm not quite sure about this one. Who do you mean by "others"? Other
Other users of DBus. For now, it's only slirp-helper, but there can be already multiple instances.
interfaces? Is this supposed to help with 3/6 so that if we attempt to double start the dbus daemon the second attempt doesn't actually kill the daemon started in the first attempt?
yes, the point is to have a single bus for the VM needs. thanks
ACK to the rest.
Michal
On 4/21/20 6:50 PM, Marc-André Lureau wrote:
Hi
On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 4/8/20 7:23 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - qemuDBusStop(driver, vm); + /* leave dbus daemon running, it may be used by others */ return -1; }
I'm not quite sure about this one. Who do you mean by "others"? Other
Other users of DBus. For now, it's only slirp-helper, but there can be already multiple instances.
Ah, I've misunderstood this part. But if there is only one user of the DBus (us who are trying to start it now), shouldn't we kill the dbus daemon? On the other hand, it will be done by qemuProcessStop() eventually. My idea was to track whether the dbus daemon is running prior starting it (basically save priv->dbusDaemonRunning before calling qemuDBusStart() and then wrap this qemuDBusStop() with 'if (was_started)'. I can do the change before push, if you are okay with it. I just like functions to tidy up on failure, maybe it's just an obsession though. Michal
Hi On Tue, Apr 21, 2020 at 7:42 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 4/21/20 6:50 PM, Marc-André Lureau wrote:
Hi
On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 4/8/20 7:23 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Don't stop the DBus daemon if a slirp helper failed to start, as it may be shared with other helpers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 09c1247892..49bffa01b8 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -355,6 +355,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - qemuDBusStop(driver, vm); + /* leave dbus daemon running, it may be used by others */ return -1; }
I'm not quite sure about this one. Who do you mean by "others"? Other
Other users of DBus. For now, it's only slirp-helper, but there can be already multiple instances.
Ah, I've misunderstood this part. But if there is only one user of the DBus (us who are trying to start it now), shouldn't we kill the dbus daemon? On the other hand, it will be done by qemuProcessStop() eventually. My idea was to track whether the dbus daemon is running prior starting it (basically save priv->dbusDaemonRunning before calling qemuDBusStart() and then wrap this qemuDBusStop() with 'if (was_started)'. I can do the change before push, if you are okay with it. I just like functions to tidy up on failure, maybe it's just an obsession though.
Oh ok, that should be fine. thanks
From: Marc-André Lureau <marcandre.lureau@redhat.com> The slirp helper process should be associated with the VM cgroup, like other helpers. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_extdevice.c | 8 ++++++++ src/qemu/qemu_slirp.c | 14 ++++++++++++++ src/qemu/qemu_slirp.h | 3 +++ 3 files changed, 25 insertions(+) diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 5a31b4d66e..dae3524307 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -284,6 +284,14 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, return -1; } + for (i = 0; i < def->nnets; i++) { + virDomainNetDefPtr net = def->nets[i]; + qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp; + + if (slirp && qemuSlirpSetupCgroup(slirp, cgroup) < 0) + return -1; + } + if (def->tpm && qemuExtTPMSetupCgroup(driver, def, cgroup) < 0) return -1; diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 49bffa01b8..71dbded607 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -242,6 +242,14 @@ qemuSlirpStop(qemuSlirpPtr slirp, } +int +qemuSlirpSetupCgroup(qemuSlirpPtr slirp, + virCgroupPtr cgroup) +{ + return virCgroupAddProcess(cgroup, slirp->pid); +} + + int qemuSlirpStart(qemuSlirpPtr slirp, virDomainObjPtr vm, @@ -249,6 +257,7 @@ qemuSlirpStart(qemuSlirpPtr slirp, virDomainNetDefPtr net, bool incoming) { + qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virCommand) cmd = NULL; g_autofree char *pidfile = NULL; @@ -348,6 +357,10 @@ qemuSlirpStart(qemuSlirpPtr slirp, } slirp->pid = pid; + + if (priv->cgroup && qemuSlirpSetupCgroup(slirp, priv->cgroup) < 0) + goto error; + return 0; error: @@ -355,6 +368,7 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); + slirp->pid = 0; /* leave dbus daemon running, it may be used by others */ return -1; } diff --git a/src/qemu/qemu_slirp.h b/src/qemu/qemu_slirp.h index 5bf9596053..e1db908814 100644 --- a/src/qemu/qemu_slirp.h +++ b/src/qemu/qemu_slirp.h @@ -75,4 +75,7 @@ void qemuSlirpStop(qemuSlirpPtr slirp, int qemuSlirpGetFD(qemuSlirpPtr slirp); +int qemuSlirpSetupCgroup(qemuSlirpPtr slirp, + virCgroupPtr cgroup); + G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuSlirp, qemuSlirpFree); -- 2.26.0.106.g9fadedd637
From: Marc-André Lureau <marcandre.lureau@redhat.com> Allow calling qemuDBusStart() multiple times (as may be done by qemu-slirp already). Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_dbus.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index f3e6f3ee37..ae55bbb299 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -177,6 +177,9 @@ qemuDBusStart(virQEMUDriverPtr driver, pid_t cpid = -1; int ret = -1; + if (priv->dbusDaemonRunning) + return 0; + if (!virFileIsExecutable(cfg->dbusDaemonName)) { virReportSystemError(errno, _("'%s' is not a suitable dbus-daemon"), -- 2.26.0.106.g9fadedd637
From: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_dbus.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index ae55bbb299..02dc3ebf07 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -142,13 +142,11 @@ qemuDBusStop(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; g_autofree char *shortName = NULL; g_autofree char *pidfile = NULL; - g_autofree char *configfile = NULL; if (!(shortName = virDomainDefGetShortName(vm->def))) return; pidfile = qemuDBusCreatePidFilename(cfg, shortName); - configfile = qemuDBusCreateConfPath(cfg, shortName); if (virPidFileForceCleanupPath(pidfile) < 0) { VIR_WARN("Unable to kill dbus-daemon process"); -- 2.26.0.106.g9fadedd637
From: Marc-André Lureau <marcandre.lureau@redhat.com> External devices are started before cgroup is created. Add the DBus daemon to the VM cgroup with the rest of the external devices. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_dbus.c | 24 ++++++++++++++++++++++++ src/qemu/qemu_dbus.h | 3 +++ src/qemu/qemu_extdevice.c | 4 ++++ 3 files changed, 31 insertions(+) diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index 02dc3ebf07..53f6c45986 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -156,6 +156,30 @@ qemuDBusStop(virQEMUDriverPtr driver, } +int +qemuDBusSetupCgroup(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autofree char *shortName = NULL; + g_autofree char *pidfile = NULL; + pid_t cpid = -1; + + if (!priv->dbusDaemonRunning) + return 0; + + if (!(shortName = virDomainDefGetShortName(vm->def))) + return -1; + pidfile = qemuDBusCreatePidFilename(cfg, shortName); + if (virPidFileReadPath(pidfile, &cpid) < 0) { + VIR_WARN("Unable to get DBus PID"); + return -1; + } + + return virCgroupAddProcess(priv->cgroup, cpid); +} + int qemuDBusStart(virQEMUDriverPtr driver, virDomainObjPtr vm) diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h index a96f19ac0d..474eb1058b 100644 --- a/src/qemu/qemu_dbus.h +++ b/src/qemu/qemu_dbus.h @@ -35,3 +35,6 @@ void qemuDBusStop(virQEMUDriverPtr driver, int qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id); void qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id); + +int qemuDBusSetupCgroup(virQEMUDriverPtr driver, + virDomainObjPtr vm); diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index dae3524307..2096272761 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -23,6 +23,7 @@ #include "qemu_command.h" #include "qemu_extdevice.h" #include "qemu_vhost_user_gpu.h" +#include "qemu_dbus.h" #include "qemu_domain.h" #include "qemu_tpm.h" #include "qemu_slirp.h" @@ -276,6 +277,9 @@ qemuExtDevicesSetupCgroup(virQEMUDriverPtr driver, virDomainDefPtr def = vm->def; size_t i; + if (qemuDBusSetupCgroup(driver, vm) < 0) + return -1; + for (i = 0; i < def->nvideos; i++) { virDomainVideoDefPtr video = def->videos[i]; -- 2.26.0.106.g9fadedd637
From: Marc-André Lureau <marcandre.lureau@redhat.com> The file doesn't use virSystemd functions directly. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_cgroup.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2afe22f177..2e019b64af 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -36,7 +36,6 @@ #include "virfile.h" #include "virtypedparam.h" #include "virnuma.h" -#include "virsystemd.h" #include "virdevmapper.h" #include "virutil.h" -- 2.26.0.106.g9fadedd637
participants (3)
-
Marc-André Lureau -
marcandre.lureau＠redhat.com
-
Michal Privoznik