On 1/14/20 2:46 PM, marcandre.lureau@xxxxxxxxxx wrote:
From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
Add a unit to start & stop a private dbus-daemon.
The daemon is meant to be started on demand, and associated with a
QEMU process. It should be stopped when the QEMU process is stopped.
The current policy is permissive like a session bus. Stricter
policies can be added later, following recommendations from:
https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus.rst
Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
---
src/qemu/Makefile.inc.am | 4 +
src/qemu/qemu_dbus.c | 299 +++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_dbus.h | 40 ++++++
src/qemu/qemu_domain.c | 2 +
src/qemu/qemu_domain.h | 3 +
tests/Makefile.am | 1 +
6 files changed, 349 insertions(+)
create mode 100644 src/qemu/qemu_dbus.c
create mode 100644 src/qemu/qemu_dbus.h
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 028ab9043c..833638ec3c 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -69,6 +69,8 @@ QEMU_DRIVER_SOURCES = \
qemu/qemu_checkpoint.h \
qemu/qemu_backup.c \
qemu/qemu_backup.h \
+ qemu/qemu_dbus.c \
+ qemu/qemu_dbus.h \
These can be added where they were just a moment ago ;-)
$(NULL)
@@ -93,6 +95,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
$(LIBNL_CFLAGS) \
$(SELINUX_CFLAGS) \
$(XDR_CFLAGS) \
+ $(DBUS_CFLAGS) \
-I$(srcdir)/access \
-I$(builddir)/access \
-I$(srcdir)/conf \
@@ -105,6 +108,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
$(GNUTLS_LIBS) \
$(LIBNL_LIBS) \
$(SELINUX_LIBS) \
+ $(DBUS_LIBS) \
$(LIBXML_LIBS) \
$(NULL)
libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
new file mode 100644
index 0000000000..9c8a03c947
--- /dev/null
+++ b/src/qemu/qemu_dbus.c
@@ -0,0 +1,299 @@
+/*
+ * qemu_dbus.c: QEMU dbus daemon
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include "qemu_extdevice.h"
+#include "qemu_dbus.h"
+#include "qemu_security.h"
+
+#include "viralloc.h"
+#include "virlog.h"
+#include "virstring.h"
+#include "virtime.h"
+#include "virpidfile.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+VIR_LOG_INIT("qemu.dbus");
+
+
+int
+qemuDBusPrepareHost(virQEMUDriverPtr driver)
+{
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+
+ return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
+ VIR_DIR_CREATE_ALLOW_EXIST);
+}
+
+
+static char *
+qemuDBusCreatePidFilename(const char *stateDir,
+ const char *shortName)
+{
+ g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
+
+ return virPidFileBuildPath(stateDir, name);
Instead of having each caller pass cfg->dbusStateDir, we can accept cfg
here and deref ourselves.
+}
+
+
+static char *
+qemuDBusCreateFilename(const char *stateDir,
+ const char *shortName,
+ const char *ext)
+{
+ g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
+
+ return virFileBuildPath(stateDir, name, ext);
+}
+
+
+static char *
+qemuDBusCreateSocketPath(virQEMUDriverConfigPtr cfg,
+ const char *shortName)
+{
+ return qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".sock");
+}
+
I'd introduce qemuDBusCreateConfPath() so that nobody else has to call
qemuDBusCreateFilename() again.
+
+char *
+qemuDBusGetAddress(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ g_autofree char *shortName = virDomainDefGetShortName(vm->def);
+ g_autofree char *path = qemuDBusCreateSocketPath(cfg, shortName);
+
+ return g_strdup_printf("unix:path=%s", path);
+}
+
+
+static int
+qemuDBusGetPid(const char *binPath,
+ const char *stateDir,
+ const char *shortName,
+ pid_t *pid)
+{
+ g_autofree char *pidfile = qemuDBusCreatePidFilename(stateDir, shortName);
+
+ return virPidFileReadPathIfAlive(pidfile, pid, binPath);
+}
After the daemon startup is reworked, this function is no longer needed.
+
+
+static int
+qemuDBusWriteConfig(const char *filename, const char *path)
+{
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ g_autofree char *config = NULL;
+
+ virBufferAddLit(&buf, "<!DOCTYPE busconfig PUBLIC \"-//freedesktop//DTD D-Bus Bus Configuration 1.0//EN\"\n");
+ virBufferAddLit(&buf, " \"http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd\";>\n");
+ virBufferAddLit(&buf, "<busconfig>\n");
+ virBufferAdjustIndent(&buf, 2);
+
+ virBufferAddLit(&buf, "<type>org.libvirt.qemu</type>\n");
+ virBufferAsprintf(&buf, "<listen>unix:path=%s</listen>\n", path);
+ virBufferAddLit(&buf, "<auth>EXTERNAL</auth>\n");
+
+ virBufferAddLit(&buf, "<policy context='default'>\n");
+ virBufferAddLit(&buf, " <!-- Allow everything to be sent -->\n");
+ virBufferAddLit(&buf, " <allow send_destination='*' eavesdrop='true'/>\n");
+ virBufferAddLit(&buf, " <!-- Allow everything to be received -->\n");
+ virBufferAddLit(&buf, " <allow eavesdrop='true'/>\n");
+ virBufferAddLit(&buf, " <!-- Allow anyone to own anything -->\n");
+ virBufferAddLit(&buf, " <allow own='*'/>\n");
'make syntax-check' complains about these leading spaces. Pff.
+ virBufferAddLit(&buf, "</policy>\n");
+
+ virBufferAddLit(&buf, "<include if_selinux_enabled='yes' selinux_root_relative='yes'>contexts/dbus_contexts</include>\n");
+
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</busconfig>\n");
+
+ config = virBufferContentAndReset(&buf);
+
+ return virFileWriteStr(filename, config, 0600);
+}
+
+
+void
+qemuDBusStop(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ g_autofree char *shortName = NULL;
+ g_autofree char *pidfile = NULL;
+ g_autofree char *configfile = NULL;
+ virErrorPtr orig_err;
+ int rc;
+ pid_t pid;
+
+ shortName = virDomainDefGetShortName(vm->def);
+ pidfile = qemuDBusCreatePidFilename(cfg->dbusStateDir, shortName);
+ configfile = qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".conf");
+
+ rc = qemuDBusGetPid(cfg->dbusDaemonName, cfg->dbusStateDir, shortName, &pid);
+ if (rc == 0 && pid != (pid_t)-1) {
+ char ebuf[1024];
+
+ VIR_DEBUG("Killing dbus-daemon process %lld", (long long)pid);
+ if (virProcessKill(pid, SIGTERM) < 0 && errno != ESRCH)
+ VIR_ERROR(_("Failed to kill process %lld: %s"),
+ (long long)pid,
+ virStrerror(errno, ebuf, sizeof(ebuf)));
+ }
+
No need to specifically kill the process. virPidFileForceCleanupPath()
does that.
+ virErrorPreserveLast(&orig_err);
+ if (virPidFileForceCleanupPath(pidfile) < 0) {
+ VIR_WARN("Unable to kill dbus-daemon process");
+ } else {
+ if (unlink(pidfile) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno,
+ _("Unable to remove stale pidfile %s"),
+ pidfile);
+ }
Just like it unlinks the pidfile.
+ }
+ if (unlink(configfile) < 0 &&
+ errno != ENOENT) {
+ virReportSystemError(errno,
+ _("Unable to remove stale configfile %s"),
+ pidfile);
+ }
+ virErrorRestore(&orig_err);
+
+ priv->dbusDaemonRunning = false;
We can do this only in success path.
+}
+
+
+int
+qemuDBusStart(virQEMUDriverPtr driver,
+ virDomainObjPtr vm)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ g_autoptr(virCommand) cmd = NULL;
+ g_autofree char *shortName = NULL;
+ g_autofree char *pidfile = NULL;
+ g_autofree char *configfile = NULL;
+ g_autofree char *sockpath = NULL;
+ virTimeBackOffVar timebackoff;
+ const unsigned long long timeout = 500 * 1000; /* ms */
+ int errfd = -1;
s/int/VIR_AUTOCLOSE/ to make sure the FD is closed when returning from
the function.
+ int cmdret = 0;
+ int exitstatus = 0;
+
+ if (priv->dbusDaemonRunning)
+ return 0;
+
+ /* for cleanup */
+ qemuDBusStop(driver, vm);
+
+ cmd = virCommandNew(cfg->dbusDaemonName);
+ shortName = virDomainDefGetShortName(vm->def);
This can fail and this the retval needs to be checked for.
+ pidfile = qemuDBusCreatePidFilename(cfg->dbusStateDir, shortName);
+ configfile = qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".conf");
+ sockpath = qemuDBusCreateSocketPath(cfg, shortName);
+
+ if (qemuDBusWriteConfig(configfile, sockpath) < 0) {
+ virReportSystemError(errno, _("Failed to write '%s'"), configfile);
+ return -1;
+ }
+
+ if (qemuSecurityDomainSetPathLabel(driver, vm, configfile, false) < 0)
+ return -1;
+
+ virCommandClearCaps(cmd);
+ virCommandSetPidFile(cmd, pidfile);
+ virCommandSetErrorFD(cmd, &errfd);
+ virCommandDaemonize(cmd);
+ virCommandAddArgFormat(cmd, "--config-file=%s", configfile);
+
+ if (qemuExtDeviceLogCommand(driver, vm, cmd, "DBus") < 0)
+ return -1;
+
+ if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1,
+ &exitstatus, &cmdret) < 0)
+ return -1;
+
+ if (cmdret < 0 || exitstatus != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not start dbus-daemon. exitstatus: %d"), exitstatus);
+ return -1;
+ }
+
+ if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0)
+ return -1;
+ while (virTimeBackOffWait(&timebackoff)) {
+ pid_t pid;
+
+ if (qemuDBusGetPid(cfg->dbusDaemonName, cfg->dbusStateDir, shortName, &pid) < 0)
+ continue;
+
There is no need to read the pid in every iteration. The virCommandRun()
(wrapped in qemuSecurityCommandRun()) will write the pidfile right
before exec(). So I think this loop should look a bit different:
while (virTimeBackOffWait(&timebackoff)) {
if (virFileExists(sockpath))
break;
if (pid still exists)
continue;
read the error;
goto cleanup;
}
Oh, and we will need cleanup label, so that in case of failure happening
after we've forked off a child, we make sure to kill it.
+ if (pid == (pid_t)-1)
+ break;
+
+ if (virFileExists(sockpath))
+ break;
+ }
+
+ if (!virFileExists(sockpath)) {
+ char errbuf[1024] = { 0 };
+
+ if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) {
+ virReportSystemError(errno, "%s", _("dbus-daemon died unexpectedly"));
+ } else {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("dbus-daemon died and reported: %s"), errbuf);
+ }
+
+ return -1;
+ }
This looks like a perfect place to put the children process into CGroup.
You define a function for that below but never call it ;-)
+
+ if (qemuSecurityDomainSetPathLabel(driver, vm, sockpath, false) < 0)
+ return -1;
+
+ priv->dbusDaemonRunning = true;
+
+ return 0;
+}
+
+
+int
+qemuDBusSetupCgroup(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virCgroupPtr cgroup)
+{
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ g_autofree char *shortName = virDomainDefGetShortName(def);
+ pid_t pid;
+ int rc;
+
+ rc = qemuDBusGetPid(cfg->dbusDaemonName, cfg->dbusStateDir, shortName, &pid);
+ if (rc < 0 || (rc == 0 && pid == (pid_t)-1)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Could not get process id of dbus-daemon"));
+ return -1;
+ }
+
+ if (virCgroupAddProcess(cgroup, pid) < 0)
+ return -1;
+
+ return 0;
+}
diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h
new file mode 100644
index 0000000000..8728824bd7
--- /dev/null
+++ b/src/qemu/qemu_dbus.h
@@ -0,0 +1,40 @@
+/*
+ * qemu_dbus.h: QEMU dbus daemon
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "qemu_conf.h"
+#include "qemu_domain.h"
+
+int qemuDBusPrepareHost(virQEMUDriverPtr driver);
+
+char *qemuDBusGetAddress(virQEMUDriverPtr driver,
+ virDomainObjPtr vm);
+
+int qemuDBusConnect(virQEMUDriverPtr driver,
+ virDomainObjPtr vm);
+
+int qemuDBusStart(virQEMUDriverPtr driver,
+ virDomainObjPtr vm);
+
+void qemuDBusStop(virQEMUDriverPtr driver,
+ virDomainObjPtr vm);
+
+int qemuDBusSetupCgroup(virQEMUDriverPtr driver,
+ virDomainDefPtr def,
+ virCgroupPtr cgroup);
Interesting, qemuDBusConnect() is declared but never defined and my gcc
doesn't complain (!).
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ecd087a5cb..7722a53c62 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2264,6 +2264,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
/* reset node name allocator */
qemuDomainStorageIdReset(priv);
+
+ priv->dbusDaemonRunning = false;
}
This shouldn't be necessary. If the qemuDBusStop() is called
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index b2041ccea7..02c792ea2a 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -40,6 +40,7 @@
#include "logging/log_manager.h"
#include "virdomainmomentobjlist.h"
#include "virenum.h"
+#include "virdbus.h"
I don't think this is needed - you are not introducing anything DBus
related in qemu_domain.h. If this is dropped then the Makefile.am change
done below can be dropped too.
#define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \
(VIR_DOMAIN_XML_SECURE)
@@ -417,6 +418,8 @@ struct _qemuDomainObjPrivate {
/* running backup job */
virDomainBackupDefPtr backup;
+
+ bool dbusDaemonRunning;
};
#define QEMU_DOMAIN_PRIVATE(vm) \
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f957c7d1ba..8ed5afbb9b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -45,6 +45,7 @@ AM_CFLAGS = \
$(YAJL_CFLAGS) \
$(COVERAGE_CFLAGS) \
$(XDR_CFLAGS) \
+ $(DBUS_CFLAGS) \
$(WARN_CFLAGS)
AM_LDFLAGS = \
Michal
