[libvirt PATCH v2 4/9] qemu: add a DBus daemon helper unit

marcandre.lureau@redhat.com posted 9 patches 5 years, 11 months ago
[libvirt PATCH v2 4/9] qemu: add a DBus daemon helper unit
Posted by marcandre.lureau@redhat.com 5 years, 11 months ago
From: Marc-André Lureau <marcandre.lureau@redhat.com>

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@redhat.com>
---
 po/POTFILES.in           |   1 +
 src/qemu/Makefile.inc.am |   2 +
 src/qemu/qemu_dbus.c     | 282 +++++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_dbus.h     |  33 +++++
 src/qemu/qemu_domain.h   |   2 +
 5 files changed, 320 insertions(+)
 create mode 100644 src/qemu/qemu_dbus.c
 create mode 100644 src/qemu/qemu_dbus.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 2d54623dc7..fe361204bb 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -150,6 +150,7 @@
 @SRCDIR@/src/qemu/qemu_checkpoint.c
 @SRCDIR@/src/qemu/qemu_command.c
 @SRCDIR@/src/qemu/qemu_conf.c
+@SRCDIR@/src/qemu/qemu_dbus.c
 @SRCDIR@/src/qemu/qemu_domain.c
 @SRCDIR@/src/qemu/qemu_domain_address.c
 @SRCDIR@/src/qemu/qemu_driver.c
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index 2b8517ecff..94a333f855 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -13,6 +13,8 @@ QEMU_DRIVER_SOURCES = \
 	qemu/qemu_capabilities.h \
 	qemu/qemu_command.c \
 	qemu/qemu_command.h \
+	qemu/qemu_dbus.c \
+	qemu/qemu_dbus.h \
 	qemu/qemu_domain.c \
 	qemu/qemu_domain.h \
 	qemu/qemu_domain_address.c \
diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
new file mode 100644
index 0000000000..383efa0209
--- /dev/null
+++ b/src/qemu/qemu_dbus.c
@@ -0,0 +1,282 @@
+/*
+ * 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)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+
+    return virDirCreate(cfg->dbusStateDir, 0770, cfg->user, cfg->group,
+                        VIR_DIR_CREATE_ALLOW_EXIST);
+}
+
+
+static char *
+qemuDBusCreatePidFilename(virQEMUDriverConfigPtr cfg,
+                          const char *shortName)
+{
+    g_autofree char *name = g_strdup_printf("%s-dbus", shortName);
+
+    return virPidFileBuildPath(cfg->dbusStateDir, name);
+}
+
+
+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");
+}
+
+
+static char *
+qemuDBusCreateConfPath(virQEMUDriverConfigPtr cfg,
+                       const char *shortName)
+{
+    return qemuDBusCreateFilename(cfg->dbusStateDir, shortName, ".conf");
+}
+
+
+char *
+qemuDBusGetAddress(virQEMUDriverPtr driver,
+                   virDomainObjPtr vm)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+    g_autofree char *shortName = virDomainDefGetShortName(vm->def);
+    g_autofree char *path = NULL;
+
+    if (!shortName)
+        return NULL;
+
+    path = qemuDBusCreateSocketPath(cfg, shortName);
+
+    return g_strdup_printf("unix:path=%s", path);
+}
+
+
+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");
+    virBufferAdjustIndent(&buf, 2);
+    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");
+    virBufferAdjustIndent(&buf, -2);
+    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)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(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");
+    } else {
+        if (unlink(configfile) < 0 &&
+            errno != ENOENT) {
+            virReportSystemError(errno,
+                                 _("Unable to remove stale configfile %s"),
+                                 pidfile);
+
+        }
+        priv->dbusDaemonRunning = false;
+    }
+}
+
+
+int
+qemuDBusStart(virQEMUDriverPtr driver,
+              virDomainObjPtr vm)
+{
+    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    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 */
+    VIR_AUTOCLOSE errfd = -1;
+    int cmdret = 0;
+    int exitstatus = 0;
+    pid_t cpid = -1;
+    int ret = -1;
+
+    if (!virFileIsExecutable(cfg->dbusDaemonName)) {
+        virReportSystemError(errno,
+                             _("'%s' is not a suitable dbus-daemon"),
+                             cfg->dbusDaemonName);
+        return -1;
+    }
+
+    if (!(shortName = virDomainDefGetShortName(vm->def)))
+        return -1;
+
+    pidfile = qemuDBusCreatePidFilename(cfg, shortName);
+    configfile = qemuDBusCreateConfPath(cfg, shortName);
+    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)
+        goto cleanup;
+
+    cmd = virCommandNew(cfg->dbusDaemonName);
+    virCommandClearCaps(cmd);
+    virCommandSetPidFile(cmd, pidfile);
+    virCommandSetErrorFD(cmd, &errfd);
+    virCommandDaemonize(cmd);
+    virCommandAddArgFormat(cmd, "--config-file=%s", configfile);
+
+    if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1,
+                               &exitstatus, &cmdret) < 0)
+        goto cleanup;
+
+    if (cmdret < 0 || exitstatus != 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Could not start dbus-daemon. exitstatus: %d"), exitstatus);
+        goto cleanup;
+    }
+
+    if (virPidFileReadPath(pidfile, &cpid) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("dbus-daemon %s didn't show up"),
+                       cfg->dbusDaemonName);
+        goto cleanup;
+    }
+
+    if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0)
+        goto cleanup;
+    while (virTimeBackOffWait(&timebackoff)) {
+        char errbuf[1024] = { 0 };
+
+        if (virFileExists(sockpath))
+            break;
+
+        if (virProcessKill(cpid, 0) == 0)
+            continue;
+
+        if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) {
+            virReportSystemError(errno,
+                                 _("dbus-daemon %s died unexpectedly"),
+                                 cfg->dbusDaemonName);
+        } else {
+            virReportError(VIR_ERR_OPERATION_FAILED,
+                           _("dbus-daemon died and reported: %s"), errbuf);
+        }
+
+        goto cleanup;
+    }
+
+    if (!virFileExists(sockpath)) {
+        virReportError(VIR_ERR_OPERATION_TIMEOUT,
+                       _("DBus daemon %s didn't show up"),
+                       cfg->dbusDaemonName);
+        goto cleanup;
+    }
+
+    if (priv->cgroup &&
+        virCgroupAddProcess(priv->cgroup, cpid) < 0)
+        goto cleanup;
+
+    if (qemuSecurityDomainSetPathLabel(driver, vm, sockpath, false) < 0)
+        goto cleanup;
+
+    priv->dbusDaemonRunning = true;
+    ret = 0;
+ cleanup:
+    if (ret < 0) {
+        virCommandAbort(cmd);
+        if (cpid >= 0)
+            virProcessKillPainfully(cpid, true);
+        unlink(pidfile);
+        unlink(configfile);
+        unlink(sockpath);
+    }
+    return ret;
+}
diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h
new file mode 100644
index 0000000000..d6cb1bc84a
--- /dev/null
+++ b/src/qemu/qemu_dbus.h
@@ -0,0 +1,33 @@
+/*
+ * 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 qemuDBusStart(virQEMUDriverPtr driver,
+                  virDomainObjPtr vm);
+
+void qemuDBusStop(virQEMUDriverPtr driver,
+                  virDomainObjPtr vm);
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index c4fd7ac302..97e52b7a81 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -417,6 +417,8 @@ struct _qemuDomainObjPrivate {
 
     /* running backup job */
     virDomainBackupDefPtr backup;
+
+    bool dbusDaemonRunning;
 };
 
 #define QEMU_DOMAIN_PRIVATE(vm) \
-- 
2.25.0.rc2.1.g09a9a1a997

Re: [libvirt PATCH v2 4/9] qemu: add a DBus daemon helper unit
Posted by Michal Privoznik 5 years, 11 months ago
On 2/25/20 10:55 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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@redhat.com>
> ---
>   po/POTFILES.in           |   1 +
>   src/qemu/Makefile.inc.am |   2 +
>   src/qemu/qemu_dbus.c     | 282 +++++++++++++++++++++++++++++++++++++++
>   src/qemu/qemu_dbus.h     |  33 +++++
>   src/qemu/qemu_domain.h   |   2 +
>   5 files changed, 320 insertions(+)
>   create mode 100644 src/qemu/qemu_dbus.c
>   create mode 100644 src/qemu/qemu_dbus.h

> diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c
> new file mode 100644
> index 0000000000..383efa0209
> --- /dev/null
> +++ b/src/qemu/qemu_dbus.c
> @@ -0,0 +1,282 @@


> +void
> +qemuDBusStop(virQEMUDriverPtr driver,
> +             virDomainObjPtr vm)
> +{
> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(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");
> +    } else {
> +        if (unlink(configfile) < 0 &&
> +            errno != ENOENT) {
> +            virReportSystemError(errno,
> +                                 _("Unable to remove stale configfile %s"),
> +                                 pidfile);
> +
> +        }

This unlink is needless as it's done by virPidFileForceCleanupPath(). 
Unfortunatelly, I might have directed you in a not so good way in my 
review of v1. I thought that virCommandSetPidFile() will cause the 
daemon to open and hold the pidfile opened. Which is not the case. 
Therefore, virPidFileForceCleanupPath() will see unlocked pidfile and 
assumes that the daemon had died meanwhle and left the pidfile behind. 
I've posted patches for that:

https://www.redhat.com/archives/libvir-list/2020-March/msg00499.html

Sorry. But once I merge them, this will start to work as expected.

> +        priv->dbusDaemonRunning = false;
> +    }
> +}
> +

Michal