[libvirt] [PATCH 0/8] Second take on slirp-helper & dbus-vmstate
From: Marc-André Lureau <marcandre.lureau@redhat.com> Hi, The series "[libvirt] [PATCH v2 00/23] Use a slirp helper process" has been merged and partially reverted. Meanwhile, qemu dbus-vmstate design has been changed and merged upstream. This new series fixes the slirp-helper support. The significant change is that dbus-vmstate now requires a bus (instead of the earlier peer-to-peer connection). The current series doesn't attempt to enforce strict policies on the bus. As long as you can connect to the bus, you can send/receive from/to anyone. A follow-up series should implement the recommendations from https://qemu.readthedocs.io/en/latest/interop/dbus.html#security. The libslirp-rs slirp-helper hasn't yet received an official release. For testing, you may: $ cargo install --features=all --git https://gitlab.freedesktop.org/slirp/libslirp-rs The resulting binary should be ~/.cargo/bin/slirp-helper, so qemu.conf slirp_helper location should be adjusted. With that in place, a VM with user networking (slirp) should now start with the helper process. thanks Marc-André Lureau (8): qemu: remove dbus-vmstate code qemu-conf: add configurable dbus-daemon location qemu-conf: add dbusStateDir qemu: add a DBus daemon helper unit domain: save/restore the state of dbus-daemon running qemu: prepare and stop the dbus daemon qemu: add dbus-vmstate helper migration support qemu-slirp: register helper for migration m4/virt-driver-qemu.m4 | 6 + src/qemu/Makefile.inc.am | 6 +- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 + src/qemu/qemu_alias.c | 17 +- src/qemu/qemu_alias.h | 3 +- src/qemu/qemu_command.c | 65 +++---- src/qemu/qemu_command.h | 6 +- src/qemu/qemu_conf.c | 9 + src/qemu/qemu_conf.h | 2 + src/qemu/qemu_dbus.c | 283 +++++++++++++++++++++++++---- src/qemu/qemu_dbus.h | 30 +-- src/qemu/qemu_domain.c | 30 +-- src/qemu/qemu_domain.h | 9 +- src/qemu/qemu_extdevice.c | 4 +- src/qemu/qemu_hotplug.c | 165 +++++++++-------- src/qemu/qemu_hotplug.h | 17 +- src/qemu/qemu_migration.c | 57 +++++- src/qemu/qemu_monitor.c | 21 +++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 15 ++ src/qemu/qemu_monitor_json.h | 5 + src/qemu/qemu_process.c | 6 + src/qemu/qemu_slirp.c | 126 ++----------- src/qemu/qemu_slirp.h | 4 +- src/qemu/test_libvirtd_qemu.aug.in | 1 + tests/Makefile.am | 1 + 27 files changed, 564 insertions(+), 331 deletions(-) -- 2.25.0.rc2.1.g09a9a1a997
From: Marc-André Lureau <marcandre.lureau@redhat.com> This code was based on a per-helper instance and peer-to-peer connections. The code that landed in qemu master for v5.0 is relying on a single instance and DBus bus. Instead of trying to adapt the existing dbus-vmstate code, let's remove it and resubmit. That should make reviewing easier. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/Makefile.inc.am | 2 - src/qemu/qemu_alias.c | 16 ----- src/qemu/qemu_alias.h | 3 - src/qemu/qemu_command.c | 83 ------------------------- src/qemu/qemu_command.h | 3 - src/qemu/qemu_dbus.c | 94 ---------------------------- src/qemu/qemu_dbus.h | 42 ------------- src/qemu/qemu_domain.c | 13 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_extdevice.c | 4 +- src/qemu/qemu_hotplug.c | 83 +------------------------ src/qemu/qemu_hotplug.h | 11 ---- src/qemu/qemu_migration.c | 8 --- src/qemu/qemu_slirp.c | 125 +------------------------------------- src/qemu/qemu_slirp.h | 4 +- 15 files changed, 7 insertions(+), 485 deletions(-) delete mode 100644 src/qemu/qemu_dbus.c delete mode 100644 src/qemu/qemu_dbus.h diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am index 967f6e75a2..028ab9043c 100644 --- a/src/qemu/Makefile.inc.am +++ b/src/qemu/Makefile.inc.am @@ -13,8 +13,6 @@ 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_alias.c b/src/qemu/qemu_alias.c index 93bdcb7548..61f8ce05c9 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -840,19 +840,3 @@ qemuDomainGetUnmanagedPRAlias(const char *parentalias) return ret; } - -char * -qemuAliasDBusVMStateFromId(const char *id) -{ - char *ret; - size_t i; - - ret = g_strdup_printf("dbus-vms-%s", id); - - for (i = 0; ret[i]; i++) { - if (ret[i] == ':') - ret[i] = '_'; - } - - return ret; -} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index ae2fce16bc..aaac09a1d1 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -95,6 +95,3 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias) const char *qemuDomainGetManagedPRAlias(void); char *qemuDomainGetUnmanagedPRAlias(const char *parentalias); - -char *qemuAliasDBusVMStateFromId(const char *id) - ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 904d2beab5..7429a0b7f5 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -27,7 +27,6 @@ #include "qemu_interface.h" #include "qemu_alias.h" #include "qemu_security.h" -#include "qemu_dbus.h" #include "qemu_slirp.h" #include "qemu_block.h" #include "cpu/cpu.h" @@ -9459,85 +9458,6 @@ qemuBuildPflashBlockdevCommandLine(virCommandPtr cmd, } -static virJSONValuePtr -qemuBuildDBusVMStateInfoPropsInternal(const char *alias, - const char *addr) -{ - virJSONValuePtr ret = NULL; - - if (qemuMonitorCreateObjectProps(&ret, - "dbus-vmstate", alias, - "s:addr", addr, NULL) < 0) - return NULL; - - return ret; -} - - -virJSONValuePtr -qemuBuildDBusVMStateInfoProps(const char *id, - const char *addr) -{ - g_autofree char *alias = qemuAliasDBusVMStateFromId(id); - - if (!alias) - return NULL; - - return qemuBuildDBusVMStateInfoPropsInternal(alias, addr); -} - - -typedef struct qemuBuildDBusVMStateCommandLineData { - virCommandPtr cmd; -} qemuBuildDBusVMStateCommandLineData; - - -static int -qemuBuildDBusVMStateCommandLineEach(void *payload, - const void *id, - void *user_data) -{ - qemuBuildDBusVMStateCommandLineData *data = user_data; - qemuDBusVMStatePtr vms = payload; - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - g_autoptr(virJSONValue) props = NULL; - - if (!(props = qemuBuildDBusVMStateInfoProps(id, vms->addr))) - return -1; - - if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0) - return -1; - - virCommandAddArg(data->cmd, "-object"); - virCommandAddArgBuffer(data->cmd, &buf); - - return 0; -} - -static int -qemuBuildDBusVMStateCommandLine(virCommandPtr cmd, - qemuDomainObjPrivatePtr priv) -{ - qemuBuildDBusVMStateCommandLineData data = { - .cmd = cmd, - }; - - if (virHashSize(priv->dbusVMStates) == 0) - return 0; - - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("dbus-vmstate object is not supported by this QEMU binary")); - return 0; - } - - if (virHashForEach(priv->dbusVMStates, qemuBuildDBusVMStateCommandLineEach, &data) < 0) - return -1; - - return 0; -} - - /** * qemuBuildCommandLineValidate: * @@ -9769,9 +9689,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) return NULL; - if (qemuBuildDBusVMStateCommandLine(cmd, priv) < 0) - return NULL; - if (qemuBuildManagedPRCommandLine(cmd, def, priv) < 0) return NULL; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 786991fd3d..cb8c394fb6 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -63,9 +63,6 @@ virJSONValuePtr qemuBuildPRManagedManagerInfoProps(qemuDomainObjPrivatePtr priv) int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, virJSONValuePtr *propsret); -virJSONValuePtr qemuBuildDBusVMStateInfoProps(const char *id, - const char *addr); - /* Generate the object properties for a tls-creds-x509 */ int qemuBuildTLSx509BackendProps(const char *tlspath, bool isListen, diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c deleted file mode 100644 index faee122021..0000000000 --- a/src/qemu/qemu_dbus.c +++ /dev/null @@ -1,94 +0,0 @@ -/* - * qemu_dbus.c: QEMU DBus-related helpers - * - * 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_hotplug.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_QEMU - -VIR_LOG_INIT("qemu.dbus"); - - -qemuDBusVMStatePtr -qemuDBusVMStateNew(const char *id, const char *addr) -{ - g_autoptr(qemuDBusVMState) self = NULL; - - if (VIR_ALLOC(self) < 0) - return NULL; - - self->id = g_strdup(id); - self->addr = g_strdup(addr); - - return g_steal_pointer(&self); -} - - -void -qemuDBusVMStateFree(qemuDBusVMStatePtr self) -{ - if (!self) - return; - - VIR_FREE(self->id); - VIR_FREE(self->addr); - VIR_FREE(self); -} - - -int -qemuDBusVMStateAdd(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *id, const char *addr, bool hot) -{ - qemuDBusVMStatePtr d = qemuDBusVMStateNew(id, addr); - qemuDomainObjPrivatePtr priv = vm->privateData; - - if (virHashAddEntry(priv->dbusVMStates, id, d) < 0) { - qemuDBusVMStateFree(d); - return -1; - } - - if (hot && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE) && - qemuDomainAttachDBusVMState(driver, vm, id, addr, QEMU_ASYNC_JOB_NONE) < 0) - return -1; - - return 0; -} - - -void -qemuDBusVMStateRemove(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *id, bool hot) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - - if (virHashRemoveEntry(priv->dbusVMStates, id) < 0 || - (hot && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE) && - qemuDomainDetachDBusVMState(driver, vm, id, QEMU_ASYNC_JOB_NONE) < 0)) - VIR_ERROR(_("Failed to remove vmstate id '%s'"), vm->def->name); -} diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h deleted file mode 100644 index ccbeb83709..0000000000 --- a/src/qemu/qemu_dbus.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * qemu_dbus.h: QEMU DBus-related helpers - * - * 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" - -typedef struct _qemuDBusVMState qemuDBusVMState; -typedef qemuDBusVMState *qemuDBusVMStatePtr; -struct _qemuDBusVMState { - char *id; - char *addr; -}; - - -qemuDBusVMStatePtr qemuDBusVMStateNew(const char *id, const char *addr); - -void qemuDBusVMStateFree(qemuDBusVMStatePtr self); - -int qemuDBusVMStateAdd(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *id, const char *addr, bool hot); - -void qemuDBusVMStateRemove(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *id, bool hot); - -G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuDBusVMState, qemuDBusVMStateFree); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a6dde15bad..ecd087a5cb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -26,7 +26,6 @@ #include "qemu_block.h" #include "qemu_cgroup.h" #include "qemu_command.h" -#include "qemu_dbus.h" #include "qemu_process.h" #include "qemu_capabilities.h" #include "qemu_hostdev.h" @@ -2164,13 +2163,6 @@ qemuDomainSetPrivatePaths(virQEMUDriverPtr driver, } -static void -dbusVMStateHashFree(void *opaque) -{ - qemuDBusVMStateFree(opaque); -} - - static void * qemuDomainObjPrivateAlloc(void *opaque) { @@ -2191,9 +2183,6 @@ qemuDomainObjPrivateAlloc(void *opaque) if (!(priv->blockjobs = virHashCreate(5, virObjectFreeHashData))) goto error; - if (!(priv->dbusVMStates = virHashCreate(5, dbusVMStateHashFree))) - goto error; - /* agent commands block by default, user can choose different behavior */ priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK; priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; @@ -2264,7 +2253,6 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) priv->migrationCaps = NULL; virHashRemoveAll(priv->blockjobs); - virHashRemoveAll(priv->dbusVMStates); virObjectUnref(priv->pflash0); priv->pflash0 = NULL; @@ -2308,7 +2296,6 @@ qemuDomainObjPrivateFree(void *data) qemuDomainMasterKeyFree(priv); virHashFree(priv->blockjobs); - virHashFree(priv->dbusVMStates); VIR_FREE(priv); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c6afc484f6..b2041ccea7 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -407,7 +407,6 @@ struct _qemuDomainObjPrivate { /* running block jobs */ virHashTablePtr blockjobs; - virHashTablePtr dbusVMStates; bool disableSlirp; /* Until we add full support for backing chains for pflash drives, these diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 463f76c21a..beb7adfe0c 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -180,7 +180,7 @@ qemuExtDevicesStart(virQEMUDriverPtr driver, qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp; if (slirp && - qemuSlirpStart(slirp, vm, driver, net, false, incomingMigration) < 0) + qemuSlirpStart(slirp, vm, driver, net, incomingMigration) < 0) return -1; } @@ -213,7 +213,7 @@ qemuExtDevicesStop(virQEMUDriverPtr driver, qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp; if (slirp) - qemuSlirpStop(slirp, vm, driver, net, false); + qemuSlirpStop(slirp, vm, driver, net); } } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 31d455505b..c5a4db3e79 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -403,83 +403,6 @@ qemuHotplugRemoveManagedPR(virQEMUDriverPtr driver, } -/** - * qemuDomainAttachDBusVMState: - * @driver: QEMU driver object - * @vm: domain object - * @id - * @addr - * @asyncJob: asynchronous job identifier - * - * Add dbus-vmstate object. - * - * Returns: 0 on success, -1 on error. - */ -int -qemuDomainAttachDBusVMState(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *id, - const char *addr, - qemuDomainAsyncJob asyncJob) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - g_autoptr(virJSONValue) props = NULL; - int ret; - - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("dbus-vmstate object is not supported by this QEMU binary")); - return -1; - } - - if (!(props = qemuBuildDBusVMStateInfoProps(id, addr))) - return -1; - - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - return -1; - - ret = qemuMonitorAddObject(priv->mon, &props, NULL); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - return -1; - - return ret; -} - - -/** - * qemuDomainDetachDBusVMState: - * @driver: QEMU driver object - * @vm: domain object - * @asyncJob: asynchronous job identifier - * - * Remove dbus-vmstate object from @vm. - * - * Returns: 0 on success, -1 on error. - */ -int -qemuDomainDetachDBusVMState(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *id, - qemuDomainAsyncJob asyncJob) -{ - qemuDomainObjPrivatePtr priv = vm->privateData; - g_autofree char *alias = qemuAliasDBusVMStateFromId(id); - int ret; - - if (!alias || - qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) - return -1; - - ret = qemuMonitorDelObject(priv->mon, alias); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - return -1; - - return ret; -} - - /** * qemuDomainChangeMediaBlockdev: * @driver: qemu driver structure @@ -1301,7 +1224,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp; if (qemuSlirpOpen(slirp, driver, vm->def) < 0 || - qemuSlirpStart(slirp, vm, driver, net, true, NULL) < 0) { + qemuSlirpStart(slirp, vm, driver, net, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to start slirp")); goto cleanup; @@ -1508,7 +1431,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virErrorPreserveLast(&originalError); netdev_name = g_strdup_printf("host%s", net->info.alias); if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) - qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver, net, true); + qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver, net); qemuDomainObjEnterMonitor(driver, vm); if (charDevPlugged && qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) @@ -4575,7 +4498,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, return -1; if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) - qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver, net, true); + qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver, net); virDomainAuditNet(vm, net, NULL, "detach", true); diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 1dfc601110..6605a6a3e0 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -151,15 +151,4 @@ int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, virBitmapPtr vcpus, bool state); -int qemuDomainAttachDBusVMState(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *id, - const char *addr, - qemuDomainAsyncJob asyncJob); - -int qemuDomainDetachDBusVMState(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *id, - qemuDomainAsyncJob asyncJob); - unsigned long long qemuDomainGetUnplugTimeout(virDomainObjPtr vm); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 29d228a8d9..71d0bb0879 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1125,7 +1125,6 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, bool remote, unsigned int flags) { - qemuDomainObjPrivatePtr priv = vm->privateData; int nsnapshots; int pauseReason; size_t i; @@ -1154,13 +1153,6 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, } } - if (virHashSize(priv->dbusVMStates) > 0 && - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain requires dbus-vmstate support")); - return false; - } - for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; qemuSlirpPtr slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp; diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 5266b36eaa..8e001f0d10 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -18,7 +18,6 @@ #include <config.h> -#include "qemu_dbus.h" #include "qemu_extdevice.h" #include "qemu_security.h" #include "qemu_slirp.h" @@ -203,48 +202,14 @@ qemuSlirpGetFD(qemuSlirpPtr slirp) } -static char * -qemuSlirpGetDBusVMStateId(virDomainNetDefPtr net) -{ - char macstr[VIR_MAC_STRING_BUFLEN] = ""; - char *id = NULL; - - /* can't use alias, because it's not stable across restarts */ - id = g_strdup_printf("slirp-%s", virMacAddrFormat(&net->mac, macstr)); - - return id; -} - - -static char * -qemuSlirpGetDBusPath(virQEMUDriverConfigPtr cfg, - const virDomainDef *def, - const char *alias) -{ - g_autofree char *shortName = NULL; - char *path = NULL; - - if (!(shortName = virDomainDefGetShortName(def))) - return NULL; - - path = g_strdup_printf("%s/%s-%s-slirp", - cfg->slirpStateDir, shortName, alias); - - return path; -} - - void qemuSlirpStop(qemuSlirpPtr slirp, virDomainObjPtr vm, virQEMUDriverPtr driver, - virDomainNetDefPtr net, - bool hot) + virDomainNetDefPtr net) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *pidfile = NULL; - g_autofree char *dbus_path = NULL; - g_autofree char *id = qemuSlirpGetDBusVMStateId(net); virErrorPtr orig_err; pid_t pid; int rc; @@ -254,12 +219,6 @@ qemuSlirpStop(qemuSlirpPtr slirp, return; } - if (id) { - qemuDBusVMStateRemove(driver, vm, id, hot); - } else { - VIR_WARN("Unable to construct vmstate id"); - } - virErrorPreserveLast(&orig_err); rc = virPidFileReadPathIfAlive(pidfile, &pid, cfg->slirpHelperName); if (rc >= 0 && pid != (pid_t) -1) @@ -273,18 +232,6 @@ qemuSlirpStop(qemuSlirpPtr slirp, } slirp->pid = 0; - dbus_path = qemuSlirpGetDBusPath(cfg, vm->def, net->info.alias); - if (dbus_path) { - if (unlink(dbus_path) < 0 && - errno != ENOENT) { - virReportSystemError(errno, - _("Unable to remove stale dbus socket %s"), - dbus_path); - } - } else { - VIR_WARN("Unable to construct dbus socket path"); - } - virErrorRestore(&orig_err); } @@ -294,17 +241,12 @@ qemuSlirpStart(qemuSlirpPtr slirp, virDomainObjPtr vm, virQEMUDriverPtr driver, virDomainNetDefPtr net, - bool hotplug, bool incoming) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virCommand) cmd = NULL; g_autofree char *pidfile = NULL; - g_autofree char *dbus_path = NULL; - g_autofree char *dbus_addr = NULL; - g_autofree char *id = NULL; size_t i; - const unsigned long long timeout = 5 * 1000; /* ms */ pid_t pid = (pid_t) -1; int rc; int exitstatus = 0; @@ -368,29 +310,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, } } - if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_P2P)) { - if (!(id = qemuSlirpGetDBusVMStateId(net))) - return -1; - - if (!(dbus_path = qemuSlirpGetDBusPath(cfg, vm->def, net->info.alias))) - return -1; - - if (unlink(dbus_path) < 0 && errno != ENOENT) { - virReportSystemError(errno, _("Unable to unlink %s"), dbus_path); - return -1; - } - - dbus_addr = g_strdup_printf("unix:path=%s", dbus_path); - - virCommandAddArgFormat(cmd, "--dbus-id=%s", id); - - virCommandAddArgFormat(cmd, "--dbus-p2p=%s", dbus_addr); - - if (incoming && - qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) - virCommandAddArg(cmd, "--dbus-incoming"); - } - if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_EXIT_WITH_PARENT)) virCommandAddArg(cmd, "--exit-with-parent"); @@ -414,46 +333,6 @@ qemuSlirpStart(qemuSlirpPtr slirp, goto error; } - if (dbus_path) { - virTimeBackOffVar timebackoff; - - if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) - goto error; - - while (virTimeBackOffWait(&timebackoff)) { - char errbuf[1024] = { 0 }; - - if (virFileExists(dbus_path)) - break; - - if (virProcessKill(pid, 0) == 0) - continue; - - if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) { - virReportSystemError(errno, - _("slirp helper %s died unexpectedly"), - cfg->prHelperName); - } else { - virReportError(VIR_ERR_OPERATION_FAILED, - _("slirp helper died and reported: %s"), errbuf); - } - goto error; - } - - if (!virFileExists(dbus_path)) { - virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", - _("slirp dbus socket did not show up")); - goto error; - } - } - - if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE) && - qemuDBusVMStateAdd(driver, vm, id, dbus_addr, hotplug) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to register slirp migration")); - goto error; - } - slirp->pid = pid; return 0; @@ -462,7 +341,5 @@ qemuSlirpStart(qemuSlirpPtr slirp, virProcessKillPainfully(pid, true); if (pidfile) unlink(pidfile); - if (dbus_path) - unlink(dbus_path); return -1; } diff --git a/src/qemu/qemu_slirp.h b/src/qemu/qemu_slirp.h index 5e80e79b15..5bf9596053 100644 --- a/src/qemu/qemu_slirp.h +++ b/src/qemu/qemu_slirp.h @@ -66,14 +66,12 @@ int qemuSlirpStart(qemuSlirpPtr slirp, virDomainObjPtr vm, virQEMUDriverPtr driver, virDomainNetDefPtr net, - bool hot, bool incoming); void qemuSlirpStop(qemuSlirpPtr slirp, virDomainObjPtr vm, virQEMUDriverPtr driver, - virDomainNetDefPtr net, - bool hot); + virDomainNetDefPtr net); int qemuSlirpGetFD(qemuSlirpPtr slirp); -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This code was based on a per-helper instance and peer-to-peer connections. The code that landed in qemu master for v5.0 is relying on a single instance and DBus bus.
Instead of trying to adapt the existing dbus-vmstate code, let's remove it and resubmit. That should make reviewing easier.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/Makefile.inc.am | 2 - src/qemu/qemu_alias.c | 16 ----- src/qemu/qemu_alias.h | 3 - src/qemu/qemu_command.c | 83 ------------------------- src/qemu/qemu_command.h | 3 - src/qemu/qemu_dbus.c | 94 ---------------------------- src/qemu/qemu_dbus.h | 42 ------------- src/qemu/qemu_domain.c | 13 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_extdevice.c | 4 +- src/qemu/qemu_hotplug.c | 83 +------------------------ src/qemu/qemu_hotplug.h | 11 ---- src/qemu/qemu_migration.c | 8 --- src/qemu/qemu_slirp.c | 125 +------------------------------------- src/qemu/qemu_slirp.h | 4 +- 15 files changed, 7 insertions(+), 485 deletions(-) delete mode 100644 src/qemu/qemu_dbus.c delete mode 100644 src/qemu/qemu_dbus.h
You missed po/POTFILES.in: @SRCDIR@/src/qemu/qemu_dbus.c Michal
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This code was based on a per-helper instance and peer-to-peer connections. The code that landed in qemu master for v5.0 is relying on a single instance and DBus bus.
Instead of trying to adapt the existing dbus-vmstate code, let's remove it and resubmit. That should make reviewing easier.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/Makefile.inc.am | 2 - src/qemu/qemu_alias.c | 16 ----- src/qemu/qemu_alias.h | 3 - src/qemu/qemu_command.c | 83 ------------------------- src/qemu/qemu_command.h | 3 - src/qemu/qemu_dbus.c | 94 ---------------------------- src/qemu/qemu_dbus.h | 42 ------------- src/qemu/qemu_domain.c | 13 ---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_extdevice.c | 4 +- src/qemu/qemu_hotplug.c | 83 +------------------------ src/qemu/qemu_hotplug.h | 11 ---- src/qemu/qemu_migration.c | 8 --- src/qemu/qemu_slirp.c | 125 +------------------------------------- src/qemu/qemu_slirp.h | 4 +- 15 files changed, 7 insertions(+), 485 deletions(-) delete mode 100644 src/qemu/qemu_dbus.c delete mode 100644 src/qemu/qemu_dbus.h
You missed po/POTFILES.in:
@SRCDIR@/src/qemu/qemu_dbus.c
yes, thanks
From: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- m4/virt-driver-qemu.m4 | 6 ++++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 +++ src/qemu/qemu_conf.c | 5 +++++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 17 insertions(+) diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index a1d2c66bba..886261fce5 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -110,6 +110,12 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [ [/usr/bin:/usr/libexec]) AC_DEFINE_UNQUOTED([QEMU_SLIRP_HELPER], ["$QEMU_SLIRP_HELPER"], [QEMU slirp helper]) + + AC_PATH_PROG([QEMU_DBUS_DAEMON], [dbus-daemon], + [/usr/bin/dbus-daemon], + [/usr/bin:/usr/libexec]) + AC_DEFINE_UNQUOTED([QEMU_DBUS_DAEMON], ["$QEMU_DBUS_DAEMON"], + [QEMU dbus daemon]) ]) AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [ diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 557b6f38f8..58a2b6416f 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -89,6 +89,7 @@ module Libvirtd_qemu = | str_entry "bridge_helper" | str_entry "pr_helper" | str_entry "slirp_helper" + | str_entry "dbus_daemon" | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index b6805ffc41..72ad4d4a10 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -828,6 +828,9 @@ # Path to the SLIRP networking helper. #slirp_helper = "/usr/bin/slirp-helper" +# Path to the dbus-daemon +#dbus_daemon = "/usr/bin/dbus-daemon" + # User for the swtpm TPM Emulator # # Default is 'tss'; this is the same user that tcsd (TrouSerS) installs diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b62dd1df52..e1fea390c7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -228,6 +228,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->bridgeHelperName = g_strdup(QEMU_BRIDGE_HELPER); cfg->prHelperName = g_strdup(QEMU_PR_HELPER); cfg->slirpHelperName = g_strdup(QEMU_SLIRP_HELPER); + cfg->slirpHelperName = g_strdup(QEMU_DBUS_DAEMON); cfg->securityDefaultConfined = true; cfg->securityRequireConfined = false; @@ -313,6 +314,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->bridgeHelperName); VIR_FREE(cfg->prHelperName); VIR_FREE(cfg->slirpHelperName); + VIR_FREE(cfg->dbusDaemonName); VIR_FREE(cfg->saveImageFormat); VIR_FREE(cfg->dumpImageFormat); @@ -604,6 +606,9 @@ virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "slirp_helper", &cfg->slirpHelperName) < 0) return -1; + if (virConfGetValueString(conf, "dbus_daemon", &cfg->dbusDaemonName) < 0) + return -1; + if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0) return -1; if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index b9401635d7..6354925e0b 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -155,6 +155,7 @@ struct _virQEMUDriverConfig { char *bridgeHelperName; char *prHelperName; char *slirpHelperName; + char *dbusDaemonName; bool macFilter; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index dd90edf687..95540a4b5e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -104,6 +104,7 @@ module Test_libvirtd_qemu = { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } { "pr_helper" = "/usr/bin/qemu-pr-helper" } { "slirp_helper" = "/usr/bin/slirp-helper" } +{ "dbus_daemon" = "/usr/bin/dbus-daemon" } { "swtpm_user" = "tss" } { "swtpm_group" = "tss" } { "capability_filters" -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- m4/virt-driver-qemu.m4 | 6 ++++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 +++ src/qemu/qemu_conf.c | 5 +++++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b62dd1df52..e1fea390c7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -228,6 +228,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->bridgeHelperName = g_strdup(QEMU_BRIDGE_HELPER); cfg->prHelperName = g_strdup(QEMU_PR_HELPER); cfg->slirpHelperName = g_strdup(QEMU_SLIRP_HELPER); + cfg->slirpHelperName = g_strdup(QEMU_DBUS_DAEMON);
Oops, s/slirpHelperName/dbusDaemonName/ :-D Michal
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- m4/virt-driver-qemu.m4 | 6 ++++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 +++ src/qemu/qemu_conf.c | 5 +++++ src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 17 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b62dd1df52..e1fea390c7 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -228,6 +228,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->bridgeHelperName = g_strdup(QEMU_BRIDGE_HELPER); cfg->prHelperName = g_strdup(QEMU_PR_HELPER); cfg->slirpHelperName = g_strdup(QEMU_SLIRP_HELPER); + cfg->slirpHelperName = g_strdup(QEMU_DBUS_DAEMON);
Oops, s/slirpHelperName/dbusDaemonName/ :-D
right, fixed
From: Marc-André Lureau <marcandre.lureau@redhat.com> Location of DBus daemon state configuration, socket, pid... Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR); + cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir); cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir); cfg->configBaseDir = virGetUserConfigDirectory(); @@ -274,6 +277,7 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->stateDir); VIR_FREE(cfg->swtpmStateDir); VIR_FREE(cfg->slirpStateDir); + VIR_FREE(cfg->dbusStateDir); VIR_FREE(cfg->libDir); VIR_FREE(cfg->cacheDir); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 6354925e0b..de6430cf97 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -97,6 +97,7 @@ struct _virQEMUDriverConfig { char *stateDir; char *swtpmStateDir; char *slirpStateDir; + char *dbusStateDir; /* These two directories are ones QEMU processes use (so must match * the QEMU user/group */ char *libDir; -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Location of DBus daemon state configuration, socket, pid...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
+ cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
cfg->configBaseDir = virGetUserConfigDirectory();
Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init. However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to: $cfg->libDir/domain-$shortName/ This way you wouldn't need to care about domain's shortname in the next patch. Michal
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Location of DBus daemon state configuration, socket, pid...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
+ cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
cfg->configBaseDir = virGetUserConfigDirectory();
Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init.
However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to:
$cfg->libDir/domain-$shortName/
This way you wouldn't need to care about domain's shortname in the next patch.
Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too?
On 2/24/20 4:57 PM, Marc-André Lureau wrote:
Hi
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Location of DBus daemon state configuration, socket, pid...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
+ cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
cfg->configBaseDir = virGetUserConfigDirectory();
Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init.
However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to:
$cfg->libDir/domain-$shortName/
This way you wouldn't need to care about domain's shortname in the next patch.
Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too?
I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir. Michal
Hi On Tue, Feb 25, 2020 at 12:24 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 2/24/20 4:57 PM, Marc-André Lureau wrote:
Hi
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Location of DBus daemon state configuration, socket, pid...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
+ cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
cfg->configBaseDir = virGetUserConfigDirectory();
Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init.
However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to:
$cfg->libDir/domain-$shortName/
This way you wouldn't need to care about domain's shortname in the next patch.
Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too?
I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir.
Actually, $cfg->libDir is virQEMUDriverConfigNew () cfg->configBaseDir/qemu/lib, so it ends up under ~/.config for users, and /etc for priviledged/root. A bad place to store transient runtime data/sockets. You may want to reconsider the paths for pr-helper. I don't intend to change that, so you could take a look at "[libvirt PATCH v2 0/9]".
On a Friday in 2020, Marc-André Lureau wrote:
Hi
On Tue, Feb 25, 2020 at 12:24 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 2/24/20 4:57 PM, Marc-André Lureau wrote:
Hi
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Location of DBus daemon state configuration, socket, pid...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
+ cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
cfg->configBaseDir = virGetUserConfigDirectory();
Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init.
However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to:
$cfg->libDir/domain-$shortName/
This way you wouldn't need to care about domain's shortname in the next patch.
Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too?
I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir.
Actually, $cfg->libDir is virQEMUDriverConfigNew () cfg->configBaseDir/qemu/lib, so it ends up under ~/.config for users, and /etc for priviledged/root. A bad place to store transient runtime
For the privileged driver, cfg->libDir is derived from LOCALSTATEDIR, so it goes in /var. Using ~/.config for unprivileged libDir does sound like an odd choice, but if that's so we should fix it for all libDir users, not keep introducing new Dirs. Jano
data/sockets. You may want to reconsider the paths for pr-helper.
I don't intend to change that, so you could take a look at "[libvirt PATCH v2 0/9]".
On 2/25/20 6:24 AM, Michal Privoznik wrote:
On 2/24/20 4:57 PM, Marc-André Lureau wrote:
Hi
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Location of DBus daemon state configuration, socket, pid...
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e1fea390c7..ade12dac6c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -144,6 +144,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
cfg->cacheDir = g_strdup_printf("%s/cache/libvirt/qemu", LOCALSTATEDIR);
+ cfg->dbusStateDir = g_strdup_printf("%s/run/libvirt/qemu/dbus", LOCALSTATEDIR); + cfg->libDir = g_strdup_printf("%s/lib/libvirt/qemu", LOCALSTATEDIR); cfg->saveDir = g_strdup_printf("%s/save", cfg->libDir); cfg->snapshotDir = g_strdup_printf("%s/snapshot", cfg->libDir); @@ -174,6 +176,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->stateDir = g_strdup_printf("%s/qemu/run", rundir);
cfg->swtpmStateDir = g_strdup_printf("%s/swtpm", cfg->stateDir); + cfg->dbusStateDir = g_strdup_printf("%s/dbus", cfg->stateDir);
cfg->configBaseDir = virGetUserConfigDirectory();
Instead of doing practically the same in two branches, you can achieve the same result with just one line if you put the call just below cfg->slirpStateDir init.
However, do we need this to be in a special directory instead of per VM private directory? The way I implemented PR helper was that the socket it creates and to which qemu connects is stored under vm->priv->libDir which is initialized in qemuDomainSetPrivatePaths() to:
$cfg->libDir/domain-$shortName/
This way you wouldn't need to care about domain's shortname in the next patch.
Makes sense. I think I based this on slirpStateDir code. Any reason it's not using vm->priv->libdir too?
I don't know. But since there are some releases which would store data under slirpStateDir I don't think we can change this. On daemon restart (with newer version) the new libvirtd wouldn't see the old dir.
It would need to be done carefully, but it's not out of the question: There was previously a case where we had been storing the network state files in an incorrect location (/var/lib/libvirt) (starting in commit 23a090ab92 - libvirt-0.6.0 in 2009), and moved them to a more proper location (/var/run/libvirt) (commit b9e95491d1 - libvirt 1.2.4 in 2014). At the time the move was made, we also added a code to networkStateInitialize() that looked for files in the old location and moved them to the new location. This code remained in place, at first doing its job whenever someone upgraded, and then silently doing nothing for a very long time, until 5 years later when it was removed in commit 43be65a4817f - libvirt 5.1.0. (probably could have been removed much earlier than that, but it had just been forgotten.)
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> --- 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 \ $(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); +} + + +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"); +} + + +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); +} + + +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"); + 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))); + } + + 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); + } + } + if (unlink(configfile) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove stale configfile %s"), + pidfile); + } + virErrorRestore(&orig_err); + + priv->dbusDaemonRunning = false; +} + + +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; + int cmdret = 0; + int exitstatus = 0; + + if (priv->dbusDaemonRunning) + return 0; + + /* for cleanup */ + qemuDBusStop(driver, vm); + + cmd = virCommandNew(cfg->dbusDaemonName); + shortName = virDomainDefGetShortName(vm->def); + 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; + + 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; + } + + 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); 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; } 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" #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 = \ -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, 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> --- 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
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, 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> --- 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!
From: Marc-André Lureau <marcandre.lureau@redhat.com> This avoids trying to start a dbus-daemon when its already running. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7722a53c62..dda3cb781f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2916,6 +2916,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainChrTypeToString(priv->monConfig->type)); } + if (priv->dbusDaemonRunning) + virBufferAddLit(buf, "<dbusDaemon/>\n"); + if (priv->namespaces) { ssize_t ns = -1; @@ -3697,6 +3700,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, goto error; } + priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 0; + if ((node = virXPathNode("./namespaces", ctxt))) { xmlNodePtr next; -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This avoids trying to start a dbus-daemon when its already running.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7722a53c62..dda3cb781f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2916,6 +2916,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainChrTypeToString(priv->monConfig->type)); }
+ if (priv->dbusDaemonRunning) + virBufferAddLit(buf, "<dbusDaemon/>\n"); + if (priv->namespaces) { ssize_t ns = -1;
@@ -3697,6 +3700,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, goto error; }
+ priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 0; + if ((node = virXPathNode("./namespaces", ctxt))) { xmlNodePtr next;
I'd push these a bit down - closer to PR daemon and slirp so that they are grouped together. Michal
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This avoids trying to start a dbus-daemon when its already running.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7722a53c62..dda3cb781f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2916,6 +2916,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainChrTypeToString(priv->monConfig->type)); }
+ if (priv->dbusDaemonRunning) + virBufferAddLit(buf, "<dbusDaemon/>\n"); + if (priv->namespaces) { ssize_t ns = -1;
@@ -3697,6 +3700,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, goto error; }
+ priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 0; + if ((node = virXPathNode("./namespaces", ctxt))) { xmlNodePtr next;
I'd push these a bit down - closer to PR daemon and slirp so that they are grouped together.
Well, as we introduce DBus bus for the VM, it would be a foundation for IPC/multi-process communication, not specific to slirp. So I'd leave it near the top.
On 2/24/20 4:58 PM, Marc-André Lureau wrote:
Hi
On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This avoids trying to start a dbus-daemon when its already running.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7722a53c62..dda3cb781f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2916,6 +2916,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainChrTypeToString(priv->monConfig->type)); }
+ if (priv->dbusDaemonRunning) + virBufferAddLit(buf, "<dbusDaemon/>\n"); + if (priv->namespaces) { ssize_t ns = -1;
@@ -3697,6 +3700,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, goto error; }
+ priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 0; + if ((node = virXPathNode("./namespaces", ctxt))) { xmlNodePtr next;
I'd push these a bit down - closer to PR daemon and slirp so that they are grouped together.
Well, as we introduce DBus bus for the VM, it would be a foundation for IPC/multi-process communication, not specific to slirp. So I'd leave it near the top.
My reasoning was that while in private data the order doesn't matter really, but so far we keep internal flags towards the beginning and helper daemons related flags towards the end. That is not to say that slirp and dbus have something in common. But apparently, our parser/formater are out of order anyway. Ideally, we should have the same order for parsing/formatting as defined in the struct. But that ship sailed long ago. Michal
From: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_process.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4195042194..845a7caa55 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -58,6 +58,7 @@ #include "qemu_extdevice.h" #include "qemu_firmware.h" #include "qemu_backup.h" +#include "qemu_dbus.h" #include "cpu/cpu.h" #include "cpu/cpu_x86.h" @@ -6457,6 +6458,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + if (qemuDBusPrepareHost(driver) < 0) + return -1; + if (qemuPrepareNVRAM(cfg, vm) < 0) return -1; @@ -7378,6 +7382,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuExtDevicesStop(driver, vm); + qemuDBusStop(driver, vm); + vm->def->id = -1; /* Stop autodestroy in case guest is restarted */ -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_process.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4195042194..845a7caa55 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -58,6 +58,7 @@ #include "qemu_extdevice.h" #include "qemu_firmware.h" #include "qemu_backup.h" +#include "qemu_dbus.h"
#include "cpu/cpu.h" #include "cpu/cpu_x86.h" @@ -6457,6 +6458,9 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
+ if (qemuDBusPrepareHost(driver) < 0) + return -1; +
Again, put this closer to qemuExtDevicesPrepareHost() so that it's close to each other.
if (qemuPrepareNVRAM(cfg, vm) < 0) return -1;
@@ -7378,6 +7382,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
qemuExtDevicesStop(driver, vm);
+ qemuDBusStop(driver, vm); + vm->def->id = -1;
/* Stop autodestroy in case guest is restarted */
Michal
From: Marc-André Lureau <marcandre.lureau@redhat.com> Helper processes may have their state migrated with QEMU data stream thanks to the QEMU "dbus-vmstate". libvirt maintains the list of helpers to be migrated. The "dbus-vmstate" is added when required, and given the list of helper Ids that must be migrated, on save & load sides. See also: https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus-vmstate.rst Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_alias.c | 7 +++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 62 +++++++++++++++++++++++++++ src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_dbus.c | 14 ++++++ src/qemu/qemu_dbus.h | 4 ++ src/qemu/qemu_domain.c | 10 +++++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_hotplug.c | 82 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 8 ++++ src/qemu/qemu_migration.c | 51 ++++++++++++++++++++++ src/qemu/qemu_monitor.c | 21 +++++++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 15 +++++++ src/qemu/qemu_monitor_json.h | 5 +++ 15 files changed, 292 insertions(+) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 61f8ce05c9..d2e1ce155e 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -840,3 +840,10 @@ qemuDomainGetUnmanagedPRAlias(const char *parentalias) return ret; } + + +const char * +qemuDomainGetDBusVMStateAlias(void) +{ + return "dbus-vmstate0"; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index aaac09a1d1..e3492116c5 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -95,3 +95,5 @@ char *qemuAliasChardevFromDevAlias(const char *devAlias) const char *qemuDomainGetManagedPRAlias(void); char *qemuDomainGetUnmanagedPRAlias(const char *parentalias); + +const char *qemuDomainGetDBusVMStateAlias(void); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7429a0b7f5..53051a8726 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -24,6 +24,7 @@ #include "qemu_command.h" #include "qemu_hostdev.h" #include "qemu_capabilities.h" +#include "qemu_dbus.h" #include "qemu_interface.h" #include "qemu_alias.h" #include "qemu_security.h" @@ -9458,6 +9459,64 @@ qemuBuildPflashBlockdevCommandLine(virCommandPtr cmd, } +static virJSONValuePtr +qemuBuildDBusVMStateInfoPropsInternal(const char *alias, + const char *addr) +{ + virJSONValuePtr ret = NULL; + + if (qemuMonitorCreateObjectProps(&ret, + "dbus-vmstate", alias, + "s:addr", addr, NULL) < 0) + return NULL; + + return ret; +} + + +virJSONValuePtr +qemuBuildDBusVMStateInfoProps(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + g_autofree char *addr = qemuDBusGetAddress(driver, vm); + + return qemuBuildDBusVMStateInfoPropsInternal(qemuDomainGetDBusVMStateAlias(), + addr); +} + + +static int +qemuBuildDBusVMStateCommandLine(virCommandPtr cmd, + virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + g_autoptr(virJSONValue) props = NULL; + qemuDomainObjPrivatePtr priv = QEMU_DOMAIN_PRIVATE(vm); + + if (virStringListLength((const char **)priv->dbusVMStateIds) == 0) + return 0; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + VIR_INFO("dbus-vmstate object is not supported by this QEMU binary"); + return 0; + } + + if (!(props = qemuBuildDBusVMStateInfoProps(driver, vm))) + return -1; + + if (virQEMUBuildObjectCommandlineFromJSON(&buf, props) < 0) + return -1; + + virCommandAddArg(cmd, "-object"); + virCommandAddArgBuffer(cmd, &buf); + + priv->dbusVMState = true; + + return 0; +} + + /** * qemuBuildCommandLineValidate: * @@ -9689,6 +9748,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) return NULL; + if (qemuBuildDBusVMStateCommandLine(cmd, driver, vm) < 0) + return NULL; + if (qemuBuildManagedPRCommandLine(cmd, def, priv) < 0) return NULL; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index cb8c394fb6..239d3daede 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -59,6 +59,9 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, virJSONValuePtr qemuBuildPRManagerInfoProps(virStorageSourcePtr src); virJSONValuePtr qemuBuildPRManagedManagerInfoProps(qemuDomainObjPrivatePtr priv); +virJSONValuePtr qemuBuildDBusVMStateInfoProps(virQEMUDriverPtr driver, + virDomainObjPtr vm); + /* Generate the object properties for a secret */ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, virJSONValuePtr *propsret); diff --git a/src/qemu/qemu_dbus.c b/src/qemu/qemu_dbus.c index 9c8a03c947..911713944b 100644 --- a/src/qemu/qemu_dbus.c +++ b/src/qemu/qemu_dbus.c @@ -297,3 +297,17 @@ qemuDBusSetupCgroup(virQEMUDriverPtr driver, return 0; } + + +int +qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id) +{ + return virStringListAdd(&QEMU_DOMAIN_PRIVATE(vm)->dbusVMStateIds, id); +} + + +void +qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id) +{ + virStringListRemove(&QEMU_DOMAIN_PRIVATE(vm)->dbusVMStateIds, id); +} diff --git a/src/qemu/qemu_dbus.h b/src/qemu/qemu_dbus.h index 8728824bd7..e86134b2a1 100644 --- a/src/qemu/qemu_dbus.h +++ b/src/qemu/qemu_dbus.h @@ -38,3 +38,7 @@ void qemuDBusStop(virQEMUDriverPtr driver, int qemuDBusSetupCgroup(virQEMUDriverPtr driver, virDomainDefPtr def, virCgroupPtr cgroup); + +int qemuDBusVMStateAdd(virDomainObjPtr vm, const char *id); + +void qemuDBusVMStateRemove(virDomainObjPtr vm, const char *id); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dda3cb781f..81dcedcc04 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2266,6 +2266,11 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) qemuDomainStorageIdReset(priv); priv->dbusDaemonRunning = false; + + virStringListFree(priv->dbusVMStateIds); + priv->dbusVMStateIds = NULL; + + priv->dbusVMState = false; } @@ -2919,6 +2924,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->dbusDaemonRunning) virBufferAddLit(buf, "<dbusDaemon/>\n"); + if (priv->dbusVMState) + virBufferAddLit(buf, "<dbusVMState/>\n"); + if (priv->namespaces) { ssize_t ns = -1; @@ -3702,6 +3710,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->dbusDaemonRunning = virXPathBoolean("boolean(./dbusDaemon)", ctxt) > 0; + priv->dbusVMState = virXPathBoolean("boolean(./dbusVMState)", ctxt) > 0; + if ((node = virXPathNode("./namespaces", ctxt))) { xmlNodePtr next; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 02c792ea2a..8de436fc9a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -420,6 +420,11 @@ struct _qemuDomainObjPrivate { virDomainBackupDefPtr backup; bool dbusDaemonRunning; + + /* list of Ids to migrate */ + char **dbusVMStateIds; + /* true if -object dbus-vmstate was added */ + bool dbusVMState; }; #define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c5a4db3e79..4563e3c77a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -311,6 +311,88 @@ qemuDomainChangeMediaLegacy(virQEMUDriverPtr driver, } +/** + * qemuHotplugAttachDBusVMState: + * @driver: QEMU driver object + * @vm: domain object + * @asyncJob: asynchronous job identifier + * + * Add -object dbus-vmstate if necessary. + * + * Returns: 0 on success, -1 on error. + */ +int +qemuHotplugAttachDBusVMState(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virJSONValue) props = NULL; + int ret; + + if (priv->dbusVMState) + return 0; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + VIR_INFO("dbus-vmstate object is not supported by this QEMU binary"); + return 0; + } + + if (!(props = qemuBuildDBusVMStateInfoProps(driver, vm))) + return -1; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + ret = qemuMonitorAddObject(priv->mon, &props, NULL); + + if (ret == 0) + priv->dbusVMState = true; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + return ret; +} + + +/** + * qemuHotplugRemoveDBusVMState: + * @driver: QEMU driver object + * @vm: domain object + * @asyncJob: asynchronous job identifier + * + * Remove -object dbus-vmstate from @vm if the configuration does not require + * it any more. + * + * Returns: 0 on success, -1 on error. + */ +int +qemuHotplugRemoveDBusVMState(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + if (!priv->dbusVMState) + return 0; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + ret = qemuMonitorDelObject(priv->mon, qemuDomainGetDBusVMStateAlias()); + + if (ret == 0) + priv->dbusVMState = false; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + return -1; + + return ret; +} + + /** * qemuHotplugAttachManagedPR: * @driver: QEMU driver object diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h index 6605a6a3e0..4a49e04a15 100644 --- a/src/qemu/qemu_hotplug.h +++ b/src/qemu/qemu_hotplug.h @@ -152,3 +152,11 @@ int qemuDomainSetVcpuInternal(virQEMUDriverPtr driver, bool state); unsigned long long qemuDomainGetUnplugTimeout(virDomainObjPtr vm); + +int qemuHotplugAttachDBusVMState(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob); + +int qemuHotplugRemoveDBusVMState(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 71d0bb0879..8c281f3a70 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1125,10 +1125,18 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, bool remote, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; int nsnapshots; int pauseReason; size_t i; + if (virStringListLength((const char **)priv->dbusVMStateIds) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot migrate this domain without dbus-vmstate support")); + return false; + } + /* perform these checks only when migrating to remote hosts */ if (remote) { nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0); @@ -1893,8 +1901,14 @@ qemuMigrationDstRun(virQEMUDriverPtr driver, if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) return -1; + rv = qemuMonitorSetDBusVMStateIdList(priv->mon, + (const char **)priv->dbusVMStateIds); + if (rv < 0) + goto exit_monitor; + rv = qemuMonitorMigrateIncoming(priv->mon, uri); + exit_monitor: if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) return -1; @@ -3369,6 +3383,37 @@ qemuMigrationSrcContinue(virQEMUDriverPtr driver, } +static int +qemuMigrationSetDBusVMState(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (virStringListLength((const char **)priv->dbusVMStateIds) > 0) { + int rv; + + if (qemuHotplugAttachDBusVMState(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + return -1; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + return -1; + + rv = qemuMonitorSetDBusVMStateIdList(priv->mon, + (const char **)priv->dbusVMStateIds); + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + rv = -1; + + return rv; + } else { + if (qemuHotplugRemoveDBusVMState(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + return -1; + } + + return 0; +} + + static int qemuMigrationSrcRun(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3521,6 +3566,9 @@ qemuMigrationSrcRun(virQEMUDriverPtr driver, } } + if (qemuMigrationSetDBusVMState(driver, vm) < 0) + goto exit_monitor; + /* Before EnterMonitor, since already qemuProcessStopCPUs does that */ if (!(flags & VIR_MIGRATE_LIVE) && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { @@ -5204,6 +5252,9 @@ qemuMigrationSrcToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, char *errbuf = NULL; virErrorPtr orig_err = NULL; + if (qemuMigrationSetDBusVMState(driver, vm) < 0) + return -1; + /* Increase migration bandwidth to unlimited since target is a file. * Failure to change migration speed is not fatal. */ if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) { diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ccd20b3740..191dbe4d02 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -27,6 +27,7 @@ #include <unistd.h> #include <fcntl.h> +#include "qemu_alias.h" #include "qemu_monitor.h" #include "qemu_monitor_text.h" #include "qemu_monitor_json.h" @@ -2407,6 +2408,26 @@ qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon, } +int +qemuMonitorSetDBusVMStateIdList(qemuMonitorPtr mon, + const char **list) +{ + g_autofree char *path = NULL; + + VIR_DEBUG("list=%p", list); + + if (virStringListLength(list) == 0) + return 0; + + path = g_strdup_printf("/objects/%s", + qemuDomainGetDBusVMStateAlias()); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetDBusVMStateIdList(mon, path, list); +} + + int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon, unsigned long bandwidth) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 3f3b81cddd..9d82d263c3 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -740,6 +740,9 @@ int qemuMonitorSavePhysicalMemory(qemuMonitorPtr mon, unsigned long long length, const char *path); +int qemuMonitorSetDBusVMStateIdList(qemuMonitorPtr mon, + const char **list); + int qemuMonitorSetMigrationSpeed(qemuMonitorPtr mon, unsigned long bandwidth); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e5164d218a..42795b4227 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2342,6 +2342,21 @@ qemuMonitorJSONSetMemoryStatsPeriod(qemuMonitorPtr mon, } +int +qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, + const char *vmstatepath, + const char **list) +{ + g_autofree char *str = virStringListJoin(list, ","); + qemuMonitorJSONObjectProperty prop = { + .type = QEMU_MONITOR_OBJECT_PROPERTY_STRING, + .val.str = str, + }; + + return qemuMonitorJSONSetObjectProperty(mon, vmstatepath, "id-list", &prop); +} + + /* qemuMonitorJSONQueryBlock: * @mon: Monitor pointer * diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 61f5b0061d..e7f02d1131 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -679,3 +679,8 @@ qemuMonitorJSONTransactionBackup(virJSONValuePtr actions, const char *target, const char *bitmap, qemuMonitorTransactionBackupSyncMode syncmode); + +int qemuMonitorJSONSetDBusVMStateIdList(qemuMonitorPtr mon, + const char *vmstatepath, + const char **list) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Helper processes may have their state migrated with QEMU data stream thanks to the QEMU "dbus-vmstate".
libvirt maintains the list of helpers to be migrated. The "dbus-vmstate" is added when required, and given the list of helper Ids that must be migrated, on save & load sides.
See also: https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus-vmstate.rst
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_alias.c | 7 +++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 62 +++++++++++++++++++++++++++ src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_dbus.c | 14 ++++++ src/qemu/qemu_dbus.h | 4 ++ src/qemu/qemu_domain.c | 10 +++++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_hotplug.c | 82 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 8 ++++ src/qemu/qemu_migration.c | 51 ++++++++++++++++++++++ src/qemu/qemu_monitor.c | 21 +++++++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 15 +++++++ src/qemu/qemu_monitor_json.h | 5 +++ 15 files changed, 292 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 71d0bb0879..8c281f3a70 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1125,10 +1125,18 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, bool remote, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; int nsnapshots; int pauseReason; size_t i;
+ if (virStringListLength((const char **)priv->dbusVMStateIds) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot migrate this domain without dbus-vmstate support")); + return false; + } +
This should be done in the if(!OFFLINE) a few lines below. IIUC, vmstate is runtime thing, and when doing offline migration (e.g. just copying over disks and XMLs), no qemu is involved and thus no vmstate matters.
/* perform these checks only when migrating to remote hosts */ if (remote) { nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0);
Michal
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Helper processes may have their state migrated with QEMU data stream thanks to the QEMU "dbus-vmstate".
libvirt maintains the list of helpers to be migrated. The "dbus-vmstate" is added when required, and given the list of helper Ids that must be migrated, on save & load sides.
See also: https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/dbus-vmstate.rst
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_alias.c | 7 +++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 62 +++++++++++++++++++++++++++ src/qemu/qemu_command.h | 3 ++ src/qemu/qemu_dbus.c | 14 ++++++ src/qemu/qemu_dbus.h | 4 ++ src/qemu/qemu_domain.c | 10 +++++ src/qemu/qemu_domain.h | 5 +++ src/qemu/qemu_hotplug.c | 82 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hotplug.h | 8 ++++ src/qemu/qemu_migration.c | 51 ++++++++++++++++++++++ src/qemu/qemu_monitor.c | 21 +++++++++ src/qemu/qemu_monitor.h | 3 ++ src/qemu/qemu_monitor_json.c | 15 +++++++ src/qemu/qemu_monitor_json.h | 5 +++ 15 files changed, 292 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 71d0bb0879..8c281f3a70 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1125,10 +1125,18 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver, bool remote, unsigned int flags) { + qemuDomainObjPrivatePtr priv = vm->privateData; int nsnapshots; int pauseReason; size_t i;
+ if (virStringListLength((const char **)priv->dbusVMStateIds) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot migrate this domain without dbus-vmstate support")); + return false; + } +
This should be done in the if(!OFFLINE) a few lines below. IIUC, vmstate is runtime thing, and when doing offline migration (e.g. just copying over disks and XMLs), no qemu is involved and thus no vmstate matters.
Right, thanks
/* perform these checks only when migrating to remote hosts */ if (remote) { nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0);
Michal
From: Marc-André Lureau <marcandre.lureau@redhat.com> When the helper supports DBus, connect it to the bus and set its ID. If the helper supports migration, register its ID to the list of dbus-vmstate ID to migrate, and specify --dbus-incoming when restoring the VM. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 8e001f0d10..48fc0a68c2 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -18,6 +18,7 @@ #include <config.h> +#include "qemu_dbus.h" #include "qemu_extdevice.h" #include "qemu_security.h" #include "qemu_slirp.h" @@ -202,6 +203,16 @@ qemuSlirpGetFD(qemuSlirpPtr slirp) } +static char * +qemuSlirpGetDBusVMStateId(virDomainNetDefPtr net) +{ + char macstr[VIR_MAC_STRING_BUFLEN] = ""; + + /* can't use alias, because it's not stable across restarts */ + return g_strdup_printf("slirp-%s", virMacAddrFormat(&net->mac, macstr)); +} + + void qemuSlirpStop(qemuSlirpPtr slirp, virDomainObjPtr vm, @@ -209,11 +220,14 @@ qemuSlirpStop(qemuSlirpPtr slirp, virDomainNetDefPtr net) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *id = qemuSlirpGetDBusVMStateId(net); g_autofree char *pidfile = NULL; virErrorPtr orig_err; pid_t pid; int rc; + qemuDBusVMStateRemove(vm, id); + if (!(pidfile = qemuSlirpCreatePidFilename(cfg, vm->def, net->info.alias))) { VIR_WARN("Unable to construct slirp pidfile path"); return; @@ -310,6 +324,29 @@ qemuSlirpStart(qemuSlirpPtr slirp, } } + if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_ADDRESS)) { + g_autofree char *id = qemuSlirpGetDBusVMStateId(net); + g_autofree char *dbus_addr = qemuDBusGetAddress(driver, vm); + + if (qemuDBusStart(driver, vm) < 0) + return -1; + + virCommandAddArgFormat(cmd, "--dbus-id=%s", id); + + virCommandAddArgFormat(cmd, "--dbus-address=%s", dbus_addr); + + if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) { + if (qemuDBusVMStateAdd(vm, id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register slirp migration")); + return -1; + } + if (incoming) { + virCommandAddArg(cmd, "--dbus-incoming"); + } + } + } + if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_EXIT_WITH_PARENT)) virCommandAddArg(cmd, "--exit-with-parent"); -- 2.25.0.rc2.1.g09a9a1a997
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
When the helper supports DBus, connect it to the bus and set its ID.
If the helper supports migration, register its ID to the list of dbus-vmstate ID to migrate, and specify --dbus-incoming when restoring the VM.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 8e001f0d10..48fc0a68c2 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -18,6 +18,7 @@
#include <config.h>
+#include "qemu_dbus.h" #include "qemu_extdevice.h" #include "qemu_security.h" #include "qemu_slirp.h" @@ -202,6 +203,16 @@ qemuSlirpGetFD(qemuSlirpPtr slirp) }
+static char * +qemuSlirpGetDBusVMStateId(virDomainNetDefPtr net) +{ + char macstr[VIR_MAC_STRING_BUFLEN] = ""; + + /* can't use alias, because it's not stable across restarts */ + return g_strdup_printf("slirp-%s", virMacAddrFormat(&net->mac, macstr)); +} + + void qemuSlirpStop(qemuSlirpPtr slirp, virDomainObjPtr vm, @@ -209,11 +220,14 @@ qemuSlirpStop(qemuSlirpPtr slirp, virDomainNetDefPtr net) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *id = qemuSlirpGetDBusVMStateId(net); g_autofree char *pidfile = NULL; virErrorPtr orig_err; pid_t pid; int rc;
+ qemuDBusVMStateRemove(vm, id); + if (!(pidfile = qemuSlirpCreatePidFilename(cfg, vm->def, net->info.alias))) { VIR_WARN("Unable to construct slirp pidfile path"); return; @@ -310,6 +324,29 @@ qemuSlirpStart(qemuSlirpPtr slirp, } }
+ if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_ADDRESS)) { + g_autofree char *id = qemuSlirpGetDBusVMStateId(net); + g_autofree char *dbus_addr = qemuDBusGetAddress(driver, vm); + + if (qemuDBusStart(driver, vm) < 0) + return -1; + + virCommandAddArgFormat(cmd, "--dbus-id=%s", id); + + virCommandAddArgFormat(cmd, "--dbus-address=%s", dbus_addr); + + if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) { + if (qemuDBusVMStateAdd(vm, id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register slirp migration")); + return -1; + } + if (incoming) { + virCommandAddArg(cmd, "--dbus-incoming"); + }
'make syntax-check' complains here. Michal
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
When the helper supports DBus, connect it to the bus and set its ID.
If the helper supports migration, register its ID to the list of dbus-vmstate ID to migrate, and specify --dbus-incoming when restoring the VM.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/qemu/qemu_slirp.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c index 8e001f0d10..48fc0a68c2 100644 --- a/src/qemu/qemu_slirp.c +++ b/src/qemu/qemu_slirp.c @@ -18,6 +18,7 @@
#include <config.h>
+#include "qemu_dbus.h" #include "qemu_extdevice.h" #include "qemu_security.h" #include "qemu_slirp.h" @@ -202,6 +203,16 @@ qemuSlirpGetFD(qemuSlirpPtr slirp) }
+static char * +qemuSlirpGetDBusVMStateId(virDomainNetDefPtr net) +{ + char macstr[VIR_MAC_STRING_BUFLEN] = ""; + + /* can't use alias, because it's not stable across restarts */ + return g_strdup_printf("slirp-%s", virMacAddrFormat(&net->mac, macstr)); +} + + void qemuSlirpStop(qemuSlirpPtr slirp, virDomainObjPtr vm, @@ -209,11 +220,14 @@ qemuSlirpStop(qemuSlirpPtr slirp, virDomainNetDefPtr net) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *id = qemuSlirpGetDBusVMStateId(net); g_autofree char *pidfile = NULL; virErrorPtr orig_err; pid_t pid; int rc;
+ qemuDBusVMStateRemove(vm, id); + if (!(pidfile = qemuSlirpCreatePidFilename(cfg, vm->def, net->info.alias))) { VIR_WARN("Unable to construct slirp pidfile path"); return; @@ -310,6 +324,29 @@ qemuSlirpStart(qemuSlirpPtr slirp, } }
+ if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_DBUS_ADDRESS)) { + g_autofree char *id = qemuSlirpGetDBusVMStateId(net); + g_autofree char *dbus_addr = qemuDBusGetAddress(driver, vm); + + if (qemuDBusStart(driver, vm) < 0) + return -1; + + virCommandAddArgFormat(cmd, "--dbus-id=%s", id); + + virCommandAddArgFormat(cmd, "--dbus-address=%s", dbus_addr); + + if (qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_MIGRATE)) { + if (qemuDBusVMStateAdd(vm, id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register slirp migration")); + return -1; + } + if (incoming) { + virCommandAddArg(cmd, "--dbus-incoming"); + }
'make syntax-check' complains here.
thanks again for the fix!
Hi On Tue, Jan 14, 2020 at 2:47 PM <marcandre.lureau@redhat.com> wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Hi,
The series "[libvirt] [PATCH v2 00/23] Use a slirp helper process" has been merged and partially reverted. Meanwhile, qemu dbus-vmstate design has been changed and merged upstream.
This new series fixes the slirp-helper support. The significant change is that dbus-vmstate now requires a bus (instead of the earlier peer-to-peer connection). The current series doesn't attempt to enforce strict policies on the bus. As long as you can connect to the bus, you can send/receive from/to anyone. A follow-up series should implement the recommendations from https://qemu.readthedocs.io/en/latest/interop/dbus.html#security.
The libslirp-rs slirp-helper hasn't yet received an official release. For testing, you may: $ cargo install --features=all --git https://gitlab.freedesktop.org/slirp/libslirp-rs
The resulting binary should be ~/.cargo/bin/slirp-helper, so qemu.conf slirp_helper location should be adjusted. With that in place, a VM with user networking (slirp) should now start with the helper process.
thanks
Marc-André Lureau (8): qemu: remove dbus-vmstate code qemu-conf: add configurable dbus-daemon location qemu-conf: add dbusStateDir qemu: add a DBus daemon helper unit domain: save/restore the state of dbus-daemon running qemu: prepare and stop the dbus daemon qemu: add dbus-vmstate helper migration support qemu-slirp: register helper for migration
Can I get some feedback/review on the series? Fwiw, in the past month, we have seen a new implementation emerge (https://github.com/majek/slirpnetstack) and this made me reconsider a number of CLI/capabilities things from the spec. I moved the spec on a wiki for now: https://gitlab.freedesktop.org/slirp/libslirp/-/wikis/Slirp-Helper. We are also discussing with podman developpers about it. Given that this is not frozen, and no helper was released so far, I understand that libvirt may want to back off a little. Yet, your feedback could also help shape/define the helper behaviour! So please, take a look :) thanks
m4/virt-driver-qemu.m4 | 6 + src/qemu/Makefile.inc.am | 6 +- src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 3 + src/qemu/qemu_alias.c | 17 +- src/qemu/qemu_alias.h | 3 +- src/qemu/qemu_command.c | 65 +++---- src/qemu/qemu_command.h | 6 +- src/qemu/qemu_conf.c | 9 + src/qemu/qemu_conf.h | 2 + src/qemu/qemu_dbus.c | 283 +++++++++++++++++++++++++---- src/qemu/qemu_dbus.h | 30 +-- src/qemu/qemu_domain.c | 30 +-- src/qemu/qemu_domain.h | 9 +- src/qemu/qemu_extdevice.c | 4 +- src/qemu/qemu_hotplug.c | 165 +++++++++-------- src/qemu/qemu_hotplug.h | 17 +- src/qemu/qemu_migration.c | 57 +++++- src/qemu/qemu_monitor.c | 21 +++ src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 15 ++ src/qemu/qemu_monitor_json.h | 5 + src/qemu/qemu_process.c | 6 + src/qemu/qemu_slirp.c | 126 ++----------- src/qemu/qemu_slirp.h | 4 +- src/qemu/test_libvirtd_qemu.aug.in | 1 + tests/Makefile.am | 1 + 27 files changed, 564 insertions(+), 331 deletions(-)
-- 2.25.0.rc2.1.g09a9a1a997
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Marc-André Lureau
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Sorry for late review. I will reply to individual patches with suggested changes. I have them locally as a fixup patches, so I can squash them and resend (keeping your authorship of course). You can also find them on my branch (if you want to test them): https://github.com/zippy2/libvirt/commits/qemu_dbus_review Michal
Hi On Thu, Feb 20, 2020 at 10:04 AM Michal Privoznik <mprivozn@redhat.com> wrote:
On 1/14/20 2:46 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Sorry for late review. I will reply to individual patches with suggested changes. I have them locally as a fixup patches, so I can squash them and resend (keeping your authorship of course). You can also find them on my branch (if you want to test them):
thanks for the review and the patches, I'll send a new version soon.
participants (6)
-
Ján Tomko -
Laine Stump -
Marc-André Lureau -
Marc-André Lureau -
marcandre.lureau＠redhat.com
-
Michal Privoznik