[PATCH 4/8] qemu: add a DBus daemon helper unit
Marc-André Lureau
marcandre.lureau at redhat.com
Mon Feb 24 15:57:55 UTC 2020
Hi
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> On 1/14/20 2:46 PM, marcandre.lureau at redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau at 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 at redhat.com>
> > ---
> > 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 ;-)
>
yep
> > $(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.
sure
>
> > +}
> > +
> > +
> > +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.
ok
>
> > +
> > +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.
oh.. ok
>
> > + 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.
ok
>
> > + 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;
> }
looks better indeed
>
>
> 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.
ok
>
> > + 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 ;-)
oops
>
> > +
> > + 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 (!).
arf, left-over
>
> > 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
Yes
>
> >
> > 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.
Yes, probably left-over 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
>
thanks a lot for the fixes & suggestions!
More information about the libvir-list
mailing list