Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote: > > 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 ;-) > 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!
