[libvirt] [RFC PATCHv2 0/4] Adding 'config' driver
This patchset adds a driver named 'config' which provides access to configuration data, such as secret and storage definitions, without requiring a connection to a hypervisor. This complements my previous patchset, which resolves the race condition on libvirtd startup. The rationale behind this idea is that there exist circumstances under which a driver may require access to things such as secrets during a time at which there is no active connection to a hypervisor. Currently, that data can not be accessed. An example of this in libvirt today is that the storage driver requires a connection in order to access secret data to auto-start an RBD storage pool that uses CephX authentication. My previous patchset breaks the ability to auto-start that type of storage pool, and this patchset fixes it again. This driver is technically what one may call a hypervisor driver, but it does not implement any domain operations. It simply exists to handle requests by drivers for access to information that would otherwise by unattainable. The URI provided by this driver is 'config:///' and has been tested working on four different machines of mine, running three different Linux distributions (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect it to work pretty much anywhere. I welcome comments and suggestions related to this driver. At the very least, this patchset combined with my previous patchset resolves the current race condition present on startup of libvirtd without any loss of functionality. Adam Walters (4): config: Adding config driver configure: Implement config driver libvirtd: Reorder load of secrets driver storage: Change hardcoded QEMU connection to use config driver configure.ac | 11 ++ daemon/libvirtd.c | 21 ++-- include/libvirt/virterror.h | 2 + po/POTFILES.in | 1 + src/Makefile.am | 25 +++++ src/config/config_driver.c | 238 +++++++++++++++++++++++++++++++++++++++++++ src/config/config_driver.h | 44 ++++++++ src/storage/storage_driver.c | 13 +-- src/util/virerror.c | 2 + 9 files changed, 345 insertions(+), 12 deletions(-) create mode 100644 src/config/config_driver.c create mode 100644 src/config/config_driver.h -- 1.8.5.2
This patch adds the source and header for a new driver, named config, which provides the 'config:///' connection URI. This driver provides access to non-domain-related configuration information, such as secret or network definitions. This helps prevent circular dependencies between drivers, including the current case between the QEMU and storage drivers. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- include/libvirt/virterror.h | 2 + src/config/config_driver.c | 238 ++++++++++++++++++++++++++++++++++++++++++++ src/config/config_driver.h | 44 ++++++++ src/util/virerror.c | 2 + 4 files changed, 286 insertions(+) create mode 100644 src/config/config_driver.c create mode 100644 src/config/config_driver.h diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index e31e9c4..0f25a65 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -121,6 +121,8 @@ typedef enum { VIR_FROM_ACCESS = 55, /* Error from access control manager */ VIR_FROM_SYSTEMD = 56, /* Error from systemd code */ + VIR_FROM_CONFIG = 57, /* Error from config driver */ + # ifdef VIR_ENUM_SENTINELS VIR_ERR_DOMAIN_LAST # endif diff --git a/src/config/config_driver.c b/src/config/config_driver.c new file mode 100644 index 0000000..c64b532 --- /dev/null +++ b/src/config/config_driver.c @@ -0,0 +1,238 @@ +/* + * config_driver.c: core driver methods for accessing configs + * + * Copyright (C) 2013 Adam Walters + * + * 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/>. + * + * Author: Adam Walters <adam@pandorasboxen.com> + */ +#include <config.h> +#include <string.h> + +#include "internal.h" +#include "datatypes.h" +#include "driver.h" +#include "virlog.h" +#include "viralloc.h" +#include "virerror.h" +#include "virstring.h" +#include "viraccessapicheck.h" +#include "nodeinfo.h" +#include "config_driver.h" + +#define VIR_FROM_THIS VIR_FROM_CONFIG + +virConfigDriverPtr config_driver = NULL; + +static int configStateCleanup(void) { + if (config_driver == NULL) + return -1; + + virObjectUnref(config_driver->closeCallbacks); + + virSysinfoDefFree(config_driver->hostsysinfo); + + virMutexDestroy(&config_driver->lock); + VIR_FREE(config_driver); + + return 0; +} + +static int configStateInitialize(bool privileged, + virStateInhibitCallback callback ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + if (!privileged) { + VIR_INFO("Not running privileged, disabling driver"); + return 0; + } + + if (VIR_ALLOC(config_driver) < 0) + return -1; + + if (virMutexInit(&config_driver->lock) < 0) { + VIR_ERROR(_("cannot initialize mutex")); + VIR_FREE(config_driver); + return -1; + } + + config_driver->hostsysinfo = virSysinfoRead(); + + if (!(config_driver->closeCallbacks = virCloseCallbacksNew())) + goto cleanup; + + return 0; + +cleanup: + configStateCleanup(); + return -1; +} + +static int configStateReload(void) { + return 0; +} + +static virDrvOpenStatus configConnectOpen(virConnectPtr conn, + virConnectAuthPtr auth ATTRIBUTE_UNUSED, + unsigned int flags) +{ + virDrvOpenStatus ret = VIR_DRV_OPEN_ERROR; + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); + + if (conn->uri != NULL) { + /* If URI isn't 'config', decline the connection */ + if (conn->uri->scheme == NULL || + STRNEQ(conn->uri->scheme, "config")) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + /* Allow remote driver to deal with URIs with hostname set */ + if (conn->uri->server != NULL) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + if (config_driver == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("config state driver is not active")); + goto cleanup; + } + } else { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + if (virConnectOpenEnsureACL(conn) < 0) + goto cleanup; + + conn->privateData = config_driver; + + ret = VIR_DRV_OPEN_SUCCESS; +cleanup: + return ret; +} + +static int configConnectClose(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + /* Cleanup callbacks on config_driver */ + /* Set conn->privateData to NULL */ + return 0; +} + +static int +configConnectSupportsFeature(virConnectPtr conn, int feature ATTRIBUTE_UNUSED) +{ + if (virConnectSupportsFeatureEnsureACL(conn) < 0) + return -1; + + return 0; +} + +static const char *configConnectGetType(virConnectPtr conn ATTRIBUTE_UNUSED) { + if (virConnectGetTypeEnsureACL(conn) < 0) + return NULL; + + return "config"; +} + +static int configConnectIsSecure(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} + +static int configConnectIsEncrypted(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 0; +} + +static char *configConnectGetHostname(virConnectPtr conn) +{ + if (virConnectGetHostnameEnsureACL(conn) < 0) + return NULL; + + return virGetHostname(); +} + +static char * +configConnectGetSysinfo(virConnectPtr conn, unsigned int flags) +{ + virConfigDriverPtr driver = conn->privateData; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + virCheckFlags(0, NULL); + + if (virConnectGetSysinfoEnsureACL(conn) < 0) + return NULL; + + if (!driver->hostsysinfo) { + return NULL; + } + + if (virSysinfoFormat(&buf, driver->hostsysinfo) < 0) + return NULL; + + if (virBufferError(&buf)) { + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +static int +configNodeGetInfo(virConnectPtr conn, + virNodeInfoPtr nodeinfo) +{ + if (virNodeGetInfoEnsureACL(conn) < 0) + return -1; + + return nodeGetInfo(nodeinfo); +} + +static int configConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) +{ + return 1; +} + +static virDriver configDriver = { + .name = "config", + .connectOpen = configConnectOpen, /* 1.2.2 */ + .connectClose = configConnectClose, /* 1.2.2 */ + .connectSupportsFeature = configConnectSupportsFeature, /* 1.2.2 */ + .connectGetType = configConnectGetType, /* 1.2.2 */ + .connectGetHostname = configConnectGetHostname, /* 1.2.2 */ + .connectGetSysinfo = configConnectGetSysinfo, /* 1.2.2 */ + .nodeGetInfo = configNodeGetInfo, /* 1.2.2 */ + .connectIsEncrypted = configConnectIsEncrypted, /* 1.2.2 */ + .connectIsSecure = configConnectIsSecure, /* 1.2.2 */ + .connectIsAlive = configConnectIsAlive, /* 1.2.2 */ +}; + + +static virStateDriver configStateDriver = { + .name = "config", + .stateInitialize = configStateInitialize, + .stateCleanup = configStateCleanup, + .stateReload = configStateReload, + .stateDrvType = VIR_DRV_STATE_DRV_LIBVIRT, +}; + +int configRegister(void) { + virRegisterDriver(&configDriver); + virRegisterStateDriver(&configStateDriver); + return 0; +} diff --git a/src/config/config_driver.h b/src/config/config_driver.h new file mode 100644 index 0000000..c74af71 --- /dev/null +++ b/src/config/config_driver.h @@ -0,0 +1,44 @@ +/* + * config_driver.h: core driver methods for accessing configs + * + * Copyright (C) 2013 Adam Walters + * + * 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/>. + * + * Author: Adam Walters <adam@pandorasboxen.com> + */ + +#ifndef __CONFIG_DRIVER_H__ +# define __CONFIG_DRIVER_H__ + +# include "virsysinfo.h" +# include "virclosecallbacks.h" + +typedef struct _virConfigDriver virConfigDriver; +typedef virConfigDriver *virConfigDriverPtr; + +struct _virConfigDriver { + virMutex lock; + + /* Immutable pointer, lockless APIs*/ + virSysinfoDefPtr hostsysinfo; + + /* Immutable pointer, self-locking APIs*/ + virCloseCallbacksPtr closeCallbacks; +}; + +int configRegister(void); + +#endif /* __CONFIG_DRIVER_H__ */ diff --git a/src/util/virerror.c b/src/util/virerror.c index f0c159f..a16a517 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -124,6 +124,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "Access Manager", /* 55 */ "Systemd", + + "Config Driver", /* 57 */ ) -- 1.8.5.2
This patch completes the addition of the config driver by adding it to the configure script, Makefile, and loading the driver in libvirtd. Additionally, to prevent a make syntax-check error, I also added src/config/config_driver.c to po/POTFILES.in. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- configure.ac | 11 +++++++++++ daemon/libvirtd.c | 9 +++++++++ po/POTFILES.in | 1 + src/Makefile.am | 25 +++++++++++++++++++++++++ 4 files changed, 46 insertions(+) diff --git a/configure.ac b/configure.ac index 3a70375..f05b0e0 100644 --- a/configure.ac +++ b/configure.ac @@ -1635,6 +1635,16 @@ if test "$with_secrets" = "yes" ; then fi AM_CONDITIONAL([WITH_SECRETS], [test "$with_secrets" = "yes"]) +if test "$with_libvirtd" = "no" ; then + with_config=no +else + with_config=yes +fi +if test "$with_config" = "yes" ; then + AC_DEFINE_UNQUOTED([WITH_CONFIG], 1, [whether local config driver is enabled]) +fi +AM_CONDITIONAL([WITH_CONFIG], [test "$with_config" = "yes"]) + AC_ARG_WITH([storage-dir], [AS_HELP_STRING([--with-storage-dir], @@ -2684,6 +2694,7 @@ AC_MSG_NOTICE([ Libvirtd: $with_libvirtd]) AC_MSG_NOTICE([Interface: $with_interface]) AC_MSG_NOTICE([ macvtap: $with_macvtap]) AC_MSG_NOTICE([ virtport: $with_virtualport]) +AC_MSG_NOTICE([ config: $with_config]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Storage Drivers]) AC_MSG_NOTICE([]) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 49c42ad..1e76f5a 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -95,6 +95,9 @@ # ifdef WITH_NWFILTER # include "nwfilter/nwfilter_driver.h" # endif +# ifdef WITH_CONFIG +# include "config/config_driver.h" +# endif #endif #include "configmake.h" @@ -369,6 +372,9 @@ static void daemonInitialize(void) * If they try to open a connection for a module that * is not loaded they'll get a suitable error at that point */ +# ifdef WITH_CONFIG + virDriverLoadModule("config"); +# endif # ifdef WITH_NETWORK virDriverLoadModule("network"); # endif @@ -406,6 +412,9 @@ static void daemonInitialize(void) virDriverLoadModule("vbox"); # endif #else +# ifdef WITH_CONFIG + configRegister(); +# endif # ifdef WITH_NETWORK networkRegister(); # endif diff --git a/po/POTFILES.in b/po/POTFILES.in index 0359b2f..e14d04d 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -27,6 +27,7 @@ src/conf/snapshot_conf.c src/conf/storage_conf.c src/conf/storage_encryption_conf.c src/conf/virchrdev.c +src/config/config_driver.c src/cpu/cpu.c src/cpu/cpu_generic.c src/cpu/cpu_map.c diff --git a/src/Makefile.am b/src/Makefile.am index 7844efa..6c60aae 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -504,6 +504,7 @@ DRIVER_SOURCE_FILES = \ $(QEMU_DRIVER_SOURCES) \ $(REMOTE_DRIVER_SOURCES) \ $(SECRET_DRIVER_SOURCES) \ + $(CONFIG_DRIVER_SOURCES) \ $(STORAGE_DRIVER_SOURCES) \ $(TEST_DRIVER_SOURCES) \ $(UML_DRIVER_SOURCES) \ @@ -523,6 +524,7 @@ STATEFUL_DRIVER_SOURCE_FILES = \ $(NWFILTER_DRIVER_SOURCES) \ $(QEMU_DRIVER_SOURCES) \ $(SECRET_DRIVER_SOURCES) \ + $(CONFIG_DRIVER_SOURCES) \ $(STORAGE_DRIVER_SOURCES) \ $(UML_DRIVER_SOURCES) \ $(XEN_DRIVER_SOURCES) \ @@ -801,6 +803,9 @@ endif WITH_INTERFACE SECRET_DRIVER_SOURCES = \ secret/secret_driver.h secret/secret_driver.c +CONFIG_DRIVER_SOURCES = \ + config/config_driver.h config/config_driver.c + # Storage backend specific impls STORAGE_DRIVER_SOURCES = \ storage/storage_driver.h storage/storage_driver.c \ @@ -1388,6 +1393,25 @@ endif WITH_DRIVER_MODULES libvirt_driver_secret_la_SOURCES = $(SECRET_DRIVER_SOURCES) endif WITH_SECRETS +if WITH_CONFIG +if WITH_DRIVER_MODULES +mod_LTLIBRARIES += libvirt_driver_config.la +else ! WITH_DRIVER_MODULES +noinst_LTLIBRARIES += libvirt_driver_config.la +# Stateful, so linked to daemon instead +#libvirt_la_BUILD_LIBADD += libvirt_driver_config.la +endif ! WITH_DRIVER_MODULES +libvirt_driver_config_la_CFLAGS = \ + -I$(top_srcdir)/src/access \ + -I$(top_srcdir)/src/conf \ + $(AM_CFLAGS) +if WITH_DRIVER_MODULES +libvirt_driver_config_la_LIBADD = ../gnulib/lib/libgnu.la +libvirt_driver_config_la_LDFLAGS = -module -avoid-version $(AM_LDFLAGS) +endif WITH_DRIVER_MODULES +libvirt_driver_config_la_SOURCES = $(CONFIG_DRIVER_SOURCES) +endif WITH_CONFIG + # Needed to keep automake quiet about conditionals libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ @@ -1663,6 +1687,7 @@ EXTRA_DIST += \ $(SECURITY_DRIVER_SELINUX_SOURCES) \ $(SECURITY_DRIVER_APPARMOR_SOURCES) \ $(SECRET_DRIVER_SOURCES) \ + $(CONFIG_DRIVER_SOURCES) \ $(VBOX_DRIVER_EXTRA_DIST) \ $(VMWARE_DRIVER_SOURCES) \ $(XENXS_SOURCES) \ -- 1.8.5.2
This patch changes the order in which the drivers are loaded in libvirtd.c. Per the comment at the top of the daemonInitialize function, order is important. Since the storage driver may depend on the secrets driver, but the secrets driver can't depend on the storage driver, I moved the loading of the secrets driver to just above the storage driver. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- daemon/libvirtd.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 1e76f5a..251c2f4 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -378,15 +378,15 @@ static void daemonInitialize(void) # ifdef WITH_NETWORK virDriverLoadModule("network"); # endif +# ifdef WITH_SECRETS + virDriverLoadModule("secret"); +# endif # ifdef WITH_STORAGE virDriverLoadModule("storage"); # endif # ifdef WITH_NODE_DEVICES virDriverLoadModule("nodedev"); # endif -# ifdef WITH_SECRETS - virDriverLoadModule("secret"); -# endif # ifdef WITH_NWFILTER virDriverLoadModule("nwfilter"); # endif @@ -421,15 +421,15 @@ static void daemonInitialize(void) # ifdef WITH_INTERFACE interfaceRegister(); # endif +# ifdef WITH_SECRETS + secretRegister(); +# endif # ifdef WITH_STORAGE storageRegister(); # endif # ifdef WITH_NODE_DEVICES nodedevRegister(); # endif -# ifdef WITH_SECRETS - secretRegister(); -# endif # ifdef WITH_NWFILTER nwfilterRegister(); # endif -- 1.8.5.2
This utilizes the new config driver I am submitting to resolve the hardcoded QEMU connection string in the storage driver. With this, the storage driver no longer has a circular dependency with QEMU. Without this patch, when libvirtd is restarted, QEMU requires storage (when domains are using storage pool backings), and storage requires QEMU (for access to secrets). This causes issues during libvirtd startup. Signed-off-by: Adam Walters <adam@pandorasboxen.com> --- src/storage/storage_driver.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d753e34..a86d564 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -70,12 +70,13 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; virConnectPtr conn = NULL; - /* XXX Remove hardcoding of QEMU URI */ - if (driverState->privileged) - conn = virConnectOpen("qemu:///system"); - else - conn = virConnectOpen("qemu:///session"); - /* Ignoring NULL conn - let backends decide */ + conn = virConnectOpen("config:///"); + /* Ignoring NULL conn - let backends decide. + * As long as the config driver is built, and + * it should be, since it is default on and + * not configurable, a NULL conn should never + * happen. + */ for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; -- 1.8.5.2
On Thu, Jan 23, 2014 at 03:14:19PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' which provides access to configuration data, such as secret and storage definitions, without requiring a connection to a hypervisor. This complements my previous patchset, which resolves the race condition on libvirtd startup.
So I had an idea about one possible alternative way to deal with this. Basically have a single global virConnectPtr instance which each non-virt driver wires up its hooks to when it starts. I've not fully tested this approach, but I've got a example patch which better illustrates what I mean: diff --git a/src/datatypes.c b/src/datatypes.c index 73f17e7..9da2ff4 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -124,6 +124,26 @@ error: return NULL; } +virConnectPtr virGetGlobalConnect(void) +{ + static virConnectPtr globalconn; + virConnectPtr conn; + + if (globalconn) + return globalconn; + + if (!(conn = virGetConnect())) + return NULL; + + if (!(conn->uri = virURIParse("global:///"))) { + virObjectUnref(conn); + return NULL; + } + + globalconn = conn; + return globalconn; +} + /** * virConnectDispose: * @conn: the hypervisor connection to release diff --git a/src/datatypes.h b/src/datatypes.h index 9621c55..6806832 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -521,6 +521,8 @@ struct _virNWFilter { */ virConnectPtr virGetConnect(void); +virConnectPtr virGetGlobalConnect(void); + virDomainPtr virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..8ac2bf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -762,6 +762,7 @@ virDomainSnapshotClass; virGetConnect; virGetDomain; virGetDomainSnapshot; +virGetGlobalConnect; virGetInterface; virGetNetwork; virGetNodeDevice; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9f7f946..a88ea23 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1102,6 +1102,10 @@ secretStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { char *base = NULL; + virConnectPtr conn; + + if ((conn = virGetGlobalConnect()) == NULL) + return -1; if (VIR_ALLOC(driverState) < 0) return -1; @@ -1127,6 +1131,7 @@ secretStateInitialize(bool privileged, if (loadSecrets(driverState, &driverState->secrets) < 0) goto error; + conn->secretPrivateData = driverState; secretDriverUnlock(driverState); return 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c83aa8a..017a74d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,14 +68,7 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; - virConnectPtr conn = NULL; - - /* XXX Remove hardcoding of QEMU URI */ - if (driverState->privileged) - conn = virConnectOpen("qemu:///system"); - else - conn = virConnectOpen("qemu:///session"); - /* Ignoring NULL conn - let backends decide */ + virConnectPtr conn = virGetGlobalConnect(); for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; @@ -129,8 +122,6 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } virStoragePoolObjUnlock(pool); } - - virObjectUnref(conn); } /** It is based on the similar idea that you had, that we don't actually need to have a full connection with virt drivers active. The difference with my approach is that we're not exposing a new URI externally - this hack is only visible inside libvirtd, so we're free to change it any time we want. If you think this approach will work I'll look at doing a complete patch for it for all non-virt drivers. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jan 28, 2014 at 12:07 PM, Daniel P. Berrange <berrange@redhat.com>wrote:
On Thu, Jan 23, 2014 at 03:14:19PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' which provides access to configuration data, such as secret and storage definitions, without requiring a connection to a hypervisor. This complements my previous patchset, which resolves the race condition on libvirtd startup.
So I had an idea about one possible alternative way to deal with this. Basically have a single global virConnectPtr instance which each non-virt driver wires up its hooks to when it starts. I've not fully tested this approach, but I've got a example patch which better illustrates what I mean:
diff --git a/src/datatypes.c b/src/datatypes.c index 73f17e7..9da2ff4 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -124,6 +124,26 @@ error: return NULL; }
+virConnectPtr virGetGlobalConnect(void) +{ + static virConnectPtr globalconn; + virConnectPtr conn; + + if (globalconn) + return globalconn; + + if (!(conn = virGetConnect())) + return NULL; + + if (!(conn->uri = virURIParse("global:///"))) { + virObjectUnref(conn); + return NULL; + } + + globalconn = conn; + return globalconn; +} + /** * virConnectDispose: * @conn: the hypervisor connection to release diff --git a/src/datatypes.h b/src/datatypes.h index 9621c55..6806832 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -521,6 +521,8 @@ struct _virNWFilter { */
virConnectPtr virGetConnect(void); +virConnectPtr virGetGlobalConnect(void); + virDomainPtr virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..8ac2bf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -762,6 +762,7 @@ virDomainSnapshotClass; virGetConnect; virGetDomain; virGetDomainSnapshot; +virGetGlobalConnect; virGetInterface; virGetNetwork; virGetNodeDevice; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9f7f946..a88ea23 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1102,6 +1102,10 @@ secretStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { char *base = NULL; + virConnectPtr conn; + + if ((conn = virGetGlobalConnect()) == NULL) + return -1;
if (VIR_ALLOC(driverState) < 0) return -1; @@ -1127,6 +1131,7 @@ secretStateInitialize(bool privileged, if (loadSecrets(driverState, &driverState->secrets) < 0) goto error;
+ conn->secretPrivateData = driverState; secretDriverUnlock(driverState); return 0;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c83aa8a..017a74d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,14 +68,7 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; - virConnectPtr conn = NULL; - - /* XXX Remove hardcoding of QEMU URI */ - if (driverState->privileged) - conn = virConnectOpen("qemu:///system"); - else - conn = virConnectOpen("qemu:///session"); - /* Ignoring NULL conn - let backends decide */ + virConnectPtr conn = virGetGlobalConnect();
for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; @@ -129,8 +122,6 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } virStoragePoolObjUnlock(pool); } - - virObjectUnref(conn); }
/**
It is based on the similar idea that you had, that we don't actually need to have a full connection with virt drivers active. The difference with my approach is that we're not exposing a new URI externally - this hack is only visible inside libvirtd, so we're free to change it any time we want.
Originally, I wanted to make the 'config:///' URI only accessible from within libvirt, but I simply didn't know how to accomplish that. I certainly see no reason why a new external URI would be needed currently. The only potential problem I can foresee with an internal URI is that eventually, if and when libvirt is split into multiple processes, the process may break down and require a public URI. I would think that could be dealt with if and when that bridge needs crossing, though. Unfortunately, I'm in the middle of rebuilding my lab, so I can't really test the patch currently (reduced VM capacity while machines are being rebuilt). If you need, I could probably test your patch by this weekend, though.
If you think this approach will work I'll look at doing a complete patch for it for all non-virt drivers.
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/:| |: http://libvirt.org -o- http://virt-manager.org:| |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc:|
On Wed, Jan 29, 2014 at 07:01:31AM -0500, Adam Walters wrote:
On Tue, Jan 28, 2014 at 12:07 PM, Daniel P. Berrange <berrange@redhat.com>wrote:
On Thu, Jan 23, 2014 at 03:14:19PM -0500, Adam Walters wrote:
This patchset adds a driver named 'config' which provides access to configuration data, such as secret and storage definitions, without requiring a connection to a hypervisor. This complements my previous patchset, which resolves the race condition on libvirtd startup.
So I had an idea about one possible alternative way to deal with this. Basically have a single global virConnectPtr instance which each non-virt driver wires up its hooks to when it starts. I've not fully tested this approach, but I've got a example patch which better illustrates what I mean:
diff --git a/src/datatypes.c b/src/datatypes.c index 73f17e7..9da2ff4 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -124,6 +124,26 @@ error: return NULL; }
+virConnectPtr virGetGlobalConnect(void) +{ + static virConnectPtr globalconn; + virConnectPtr conn; + + if (globalconn) + return globalconn; + + if (!(conn = virGetConnect())) + return NULL; + + if (!(conn->uri = virURIParse("global:///"))) { + virObjectUnref(conn); + return NULL; + } + + globalconn = conn; + return globalconn; +} + /** * virConnectDispose: * @conn: the hypervisor connection to release diff --git a/src/datatypes.h b/src/datatypes.h index 9621c55..6806832 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -521,6 +521,8 @@ struct _virNWFilter { */
virConnectPtr virGetConnect(void); +virConnectPtr virGetGlobalConnect(void); + virDomainPtr virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0c16343..8ac2bf5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -762,6 +762,7 @@ virDomainSnapshotClass; virGetConnect; virGetDomain; virGetDomainSnapshot; +virGetGlobalConnect; virGetInterface; virGetNetwork; virGetNodeDevice; diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 9f7f946..a88ea23 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1102,6 +1102,10 @@ secretStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { char *base = NULL; + virConnectPtr conn; + + if ((conn = virGetGlobalConnect()) == NULL) + return -1;
if (VIR_ALLOC(driverState) < 0) return -1; @@ -1127,6 +1131,7 @@ secretStateInitialize(bool privileged, if (loadSecrets(driverState, &driverState->secrets) < 0) goto error;
+ conn->secretPrivateData = driverState; secretDriverUnlock(driverState); return 0;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c83aa8a..017a74d 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -68,14 +68,7 @@ static void storageDriverUnlock(virStorageDriverStatePtr driver) static void storageDriverAutostart(virStorageDriverStatePtr driver) { size_t i; - virConnectPtr conn = NULL; - - /* XXX Remove hardcoding of QEMU URI */ - if (driverState->privileged) - conn = virConnectOpen("qemu:///system"); - else - conn = virConnectOpen("qemu:///session"); - /* Ignoring NULL conn - let backends decide */ + virConnectPtr conn = virGetGlobalConnect();
for (i = 0; i < driver->pools.count; i++) { virStoragePoolObjPtr pool = driver->pools.objs[i]; @@ -129,8 +122,6 @@ storageDriverAutostart(virStorageDriverStatePtr driver) { } virStoragePoolObjUnlock(pool); } - - virObjectUnref(conn); }
/**
It is based on the similar idea that you had, that we don't actually need to have a full connection with virt drivers active. The difference with my approach is that we're not exposing a new URI externally - this hack is only visible inside libvirtd, so we're free to change it any time we want.
Originally, I wanted to make the 'config:///' URI only accessible from within libvirt, but I simply didn't know how to accomplish that. I certainly see no reason why a new external URI would be needed currently. The only potential problem I can foresee with an internal URI is that eventually, if and when libvirt is split into multiple processes, the process may break down and require a public URI. I would think that could be dealt with if and when that bridge needs crossing, though.
Unfortunately, I'm in the middle of rebuilding my lab, so I can't really test the patch currently (reduced VM capacity while machines are being rebuilt). If you need, I could probably test your patch by this weekend, though.
Ah don't worry about it too much, as I'm off to FOSDEM on friday. I'll continue to explore this idea and see about reating a full patch to test next week. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Adam Walters -
Daniel P. Berrange