12:23 p.m.
From: Marc-André Lureau <marcandre.lureau(a)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
12:23 p.m.
New subject: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
From: Marc-André Lureau <marcandre.lureau(a)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(a)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
11:04 a.m.
New subject: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
On 4/8/20 7:23 PM, marcandre.lureau(a)redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau(a)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(a)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
11:50 a.m.
New subject: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
Hi
On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik <mprivozn(a)redhat.com> wrote:
On 4/8/20 7:23 PM, marcandre.lureau(a)redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau(a)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(a)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
12:41 p.m.
New subject: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
On 4/21/20 6:50 PM, Marc-André Lureau wrote:
Hi
On Tue, Apr 21, 2020 at 6:04 PM Michal Privoznik <mprivozn(a)redhat.com> wrote:
>
> On 4/8/20 7:23 PM, marcandre.lureau(a)redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau(a)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(a)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
1:10 p.m.
New subject: [libvirt PATCH 1/6] slirp: leave the dbus daemon running on error
Hi
On Tue, Apr 21, 2020 at 7:42 PM Michal Privoznik <mprivozn(a)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(a)redhat.com> wrote:
>>
>> On 4/8/20 7:23 PM, marcandre.lureau(a)redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau(a)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(a)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
12:23 p.m.
New subject: [libvirt PATCH 2/6] slirp: add helper to VM cgroup
From: Marc-André Lureau <marcandre.lureau(a)redhat.com>
The slirp helper process should be associated with the VM cgroup, like
other helpers.
Signed-off-by: Marc-André Lureau <marcandre.lureau(a)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
12:23 p.m.
New subject: [libvirt PATCH 3/6] qemu-dbus: prevent double start of the bus daemon
From: Marc-André Lureau <marcandre.lureau(a)redhat.com>
Allow calling qemuDBusStart() multiple times (as may be done by
qemu-slirp already).
Signed-off-by: Marc-André Lureau <marcandre.lureau(a)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
12:23 p.m.
New subject: [libvirt PATCH 4/6] qemu-dbus: remove unused variable
From: Marc-André Lureau <marcandre.lureau(a)redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau(a)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
12:23 p.m.
New subject: [libvirt PATCH 5/6] qemu-dbus: register DBus bus to the VM cgroup
From: Marc-André Lureau <marcandre.lureau(a)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(a)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
12:23 p.m.
New subject: [libvirt PATCH 6/6] qemu-cgroup: remove unnecessary include
From: Marc-André Lureau <marcandre.lureau(a)redhat.com>
The file doesn't use virSystemd functions directly.
Signed-off-by: Marc-André Lureau <marcandre.lureau(a)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
1678
days inactive
1692
days old
11 comments
3 participants
participants (3)
-
Marc-André Lureau -
marcandre.lureau＠redhat.com
-
Michal Privoznik