[libvirt] [PATCH 00/14] Initial split of the daemons

This patch series is the final bit of refactoring needed to start splitting up libvirtd for real. The last three patches are not really quite ready for merge, but I've included them to illustrate what the end result is looking like. I have taken the approach of adding conditionals into the libvirtd source, so that we don't end up with 10+ cut+paste copies of all the boilerplate code. Essentially the virt${DRIVER}d daemons are functionally identical to libvirtd, except that anything todo with TCP is disabled, and they only load a single driver. This does mean they are somewhat "fatter" than they need to be (mostly in terms of number of worker threads in every daemon), but I figure we can optimize later if desired. Obviously splitting off storage, nwfilter and network drivers is going to require extra work first because of the callbacks they have into virt drivers. Hence I've started with the secret driver as the trivial thing to prove the general concept. The nodedev and interface drivers will also be trivial, at least to start with. I'm contemplating whether we should put the host device assignment tracking into the nodedev driver itself. With this series, if you connect to the QEMU driver, you should automatically get a connection to the separate virtsecretd daemon. Similarly when the QEMU driver tries to open the secret driver, it should end up talking to the virtsecretd daemon via the remote driver. Daniel P. Berrangé (14): build: prevent unloading of all public libraries remote: stop trying to load Xen driver module build: prevent unloading of dlopen'd modules driver: don't keep a pointer to the loaded library handle driver: fix handling of error return from finding resource driver: tighten check for whether loadable module exists or not driver: use normal error reporting APIs when loading modules driver: add option to make missing drivers a fatal problem remote: honour errors from virDriverLoadModule remote: split URI scheme into driver and transport upfront remote: refactor code for building UNIX socket paths remote: conditionalize sources for some pieces to become optional remote: allow remote driver to connect to alternative daemons secret: introduce virtsecretd daemon src/Makefile.am | 12 ++- src/driver.c | 91 ++++++++++------ src/driver.h | 9 +- src/libvirt.c | 24 +++++ src/remote/Makefile.inc.am | 20 ++++ src/remote/remote_daemon.c | 196 ++++++++++++++++++++-------------- src/remote/remote_daemon_config.c | 36 +++++-- src/remote/remote_daemon_config.h | 9 +- src/remote/remote_daemon_dispatch.c | 11 +- src/remote/remote_driver.c | 202 ++++++++++++++++++++++++------------ src/remote/remote_driver.h | 4 - src/secret/Makefile.inc.am | 54 ++++++++++ src/storage/storage_backend.c | 13 +-- src/util/virfile.c | 4 + tests/virdrivermoduletest.c | 2 +- 15 files changed, 483 insertions(+), 204 deletions(-) -- 2.14.3

We previously added "-z nodelete" to the build of libvirt.so to prevent crashes when thread local destructors run which point to a code that has been dlclose()d: commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 19 11:42:22 2018 +0100 driver: prevent unloading of dlopen'd modules We forgot to copy this protection into the libvirt-qemu.so, libvirt-lxc.so and libvirt-admin.so libraries when we introduced them. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index a6667228b6..2f8e5f6908 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -529,7 +529,9 @@ libvirt_admin_la_SOURCES = \ libvirt_admin_la_LDFLAGS = \ $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_ADMIN_SYMBOL_FILE) \ -version-info $(LIBVIRT_VERSION_INFO) \ - $(AM_LDFLAGS) + $(LIBVIRT_NODELETE) \ + $(AM_LDFLAGS) \ + $(NULL) libvirt_admin_la_LIBADD = \ libvirt.la \ @@ -644,6 +646,7 @@ libvirt_qemu_la_SOURCES = libvirt-qemu.c libvirt_qemu_la_LDFLAGS = \ $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \ -version-info $(LIBVIRT_VERSION_INFO) \ + $(LIBVIRT_NODELETE) \ $(AM_LDFLAGS) \ $(NULL) libvirt_qemu_la_CFLAGS = $(AM_CFLAGS) @@ -653,6 +656,7 @@ libvirt_lxc_la_SOURCES = libvirt-lxc.c libvirt_lxc_la_LDFLAGS = \ $(VERSION_SCRIPT_FLAGS)$(LIBVIRT_LXC_SYMBOL_FILE) \ -version-info $(LIBVIRT_VERSION_INFO) \ + $(LIBVIRT_NODELETE) \ $(AM_LDFLAGS) \ $(NULL) libvirt_lxc_la_CFLAGS = $(AM_CFLAGS) -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
We previously added "-z nodelete" to the build of libvirt.so to prevent crashes when thread local destructors run which point to a code that has been dlclose()d:
commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 19 11:42:22 2018 +0100
driver: prevent unloading of dlopen'd modules
This commit does not exist. However 8e44e5593eb does.
We forgot to copy this protection into the libvirt-qemu.so, libvirt-lxc.so and libvirt-admin.so libraries when we introduced them.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
ACK Michal

On Tue, Apr 24, 2018 at 02:17:00PM +0200, Michal Privoznik wrote:
On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
We previously added "-z nodelete" to the build of libvirt.so to prevent crashes when thread local destructors run which point to a code that has been dlclose()d:
commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 19 11:42:22 2018 +0100
driver: prevent unloading of dlopen'd modules
This commit does not exist. However 8e44e5593eb does.
Yes, yes, copied in a local commit by mistake - the hash you mention is indeed the right one.
We forgot to copy this protection into the libvirt-qemu.so, libvirt-lxc.so and libvirt-admin.so libraries when we introduced them.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
ACK
Michal
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The Xen driver was recently deleted, but libvirtd has left over code that tries to use it. Fortunately this is dead code because WITH_XEN will never be defined anymore. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 31c6ce1b61..3e02297eee 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -328,9 +328,6 @@ static void daemonInitialize(void) #ifdef WITH_NWFILTER VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter"); #endif -#ifdef WITH_XEN - VIR_DAEMON_LOAD_MODULE(xenRegister, "xen"); -#endif #ifdef WITH_LIBXL VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl"); #endif -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
The Xen driver was recently deleted, but libvirtd has left over code that tries to use it. Fortunately this is dead code because WITH_XEN will never be defined anymore.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 31c6ce1b61..3e02297eee 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -328,9 +328,6 @@ static void daemonInitialize(void) #ifdef WITH_NWFILTER VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter"); #endif -#ifdef WITH_XEN - VIR_DAEMON_LOAD_MODULE(xenRegister, "xen"); -#endif #ifdef WITH_LIBXL VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl"); #endif
ACK please. Also might be worth removing the following from virsh (in a separate patch perhaps?) diff --git i/tools/virsh.c w/tools/virsh.c index 5f8352e861..62226eea4c 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -526,9 +526,6 @@ virshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #ifdef WITH_UML vshPrint(ctl, " UML"); #endif -#ifdef WITH_XEN - vshPrint(ctl, " Xen"); -#endif #ifdef WITH_LIBXL vshPrint(ctl, " LibXL"); #endif Michal

On Tue, Apr 24, 2018 at 02:16:59PM +0200, Michal Privoznik wrote:
On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
The Xen driver was recently deleted, but libvirtd has left over code that tries to use it. Fortunately this is dead code because WITH_XEN will never be defined anymore.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 31c6ce1b61..3e02297eee 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -328,9 +328,6 @@ static void daemonInitialize(void) #ifdef WITH_NWFILTER VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter"); #endif -#ifdef WITH_XEN - VIR_DAEMON_LOAD_MODULE(xenRegister, "xen"); -#endif #ifdef WITH_LIBXL VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl"); #endif
ACK please. Also might be worth removing the following from virsh (in a separate patch perhaps?)
Yes, I'll push that as a trivial patch too
diff --git i/tools/virsh.c w/tools/virsh.c index 5f8352e861..62226eea4c 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -526,9 +526,6 @@ virshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) #ifdef WITH_UML vshPrint(ctl, " UML"); #endif -#ifdef WITH_XEN - vshPrint(ctl, " Xen"); -#endif #ifdef WITH_LIBXL vshPrint(ctl, " LibXL"); #endif
Michal
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

We previously added "-z nodelete" to the build of libvirt.so to prevent crashes when thread local destructors run which point to a code that has been dlclose()d: commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 19 11:42:22 2018 +0100 driver: prevent unloading of dlopen'd modules The libvirtd loadable modules can suffer from the same problem if they were ever unloaded. Fortunately we don't ever call dlclose() on them, but lets add a second layer of protection by linking them with the "-z nodelete" flag. While we're doing this, lets add a third layer of protection by passing RTLD_NODELETE to dlopen(). Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 6 +++++- src/driver.c | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 2f8e5f6908..0d8d380df1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -51,7 +51,11 @@ AM_LDFLAGS = $(DRIVER_MODULES_LDFLAGS) \ $(CYGWIN_EXTRA_LDFLAGS) \ $(MINGW_EXTRA_LDFLAGS) \ $(NULL) -AM_LDFLAGS_MOD = -module -avoid-version $(AM_LDFLAGS) +AM_LDFLAGS_MOD = \ + -module \ + -avoid-version \ + $(LIBVIRT_NODELETE) \ + $(AM_LDFLAGS) AM_LDFLAGS_MOD_NOUNDEF = $(AM_LDFLAGS_MOD) $(NO_UNDEFINED_LDFLAGS) POD2MAN = pod2man -c "Virtualization Support" -r "$(PACKAGE)-$(VERSION)" diff --git a/src/driver.c b/src/driver.c index 52e1ae345a..7d4c78eaaa 100644 --- a/src/driver.c +++ b/src/driver.c @@ -45,6 +45,11 @@ static void * virDriverLoadModuleFile(const char *file) { void *handle = NULL; + int flags = RTLD_NOW | RTLD_GLOBAL; + +# ifdef RTLD_NODELETE + flags |= RTLD_NODELETE; +# endif VIR_DEBUG("Load module file '%s'", file); @@ -55,7 +60,7 @@ virDriverLoadModuleFile(const char *file) virUpdateSelfLastChanged(file); - if (!(handle = dlopen(file, RTLD_NOW | RTLD_GLOBAL))) + if (!(handle = dlopen(file, flags))) VIR_ERROR(_("failed to load module %s %s"), file, dlerror()); return handle; -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
We previously added "-z nodelete" to the build of libvirt.so to prevent crashes when thread local destructors run which point to a code that has been dlclose()d:
commit 384b9a76a5e387f64cfe8f83f4a518bb302e80f7 Author: Daniel P. Berrangé <berrange@redhat.com> Date: Thu Apr 19 11:42:22 2018 +0100
driver: prevent unloading of dlopen'd modules
As I've pointed earlier no such commit exists.
The libvirtd loadable modules can suffer from the same problem if they were ever unloaded. Fortunately we don't ever call dlclose() on them, but lets add a second layer of protection by linking them with the "-z nodelete" flag. While we're doing this, lets add a third layer of protection by passing RTLD_NODELETE to dlopen().
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/Makefile.am | 6 +++++- src/driver.c | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-)
ACK Michal

Now that we've activated two hacks to prevent unloading of modules, there is no point passing back a pointer to the loaded library handle. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 23 +++++++---------------- src/driver.h | 5 ++--- src/storage/storage_backend.c | 2 +- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/driver.c b/src/driver.c index 7d4c78eaaa..4d314b3870 100644 --- a/src/driver.c +++ b/src/driver.c @@ -86,12 +86,11 @@ virDriverLoadModuleFunc(void *handle, * virDriverLoadModuleFull: * @path: filename of module to load * @regfunc: name of the function that registers the module - * @handle: Returns handle of the loaded library if not NULL * * Loads a loadable module named @path and calls the - * registration function @regfunc. If @handle is not NULL the handle is returned - * in the variable. Otherwise the handle is leaked so that the module stays - * loaded forever. + * registration function @regfunc. The module will never + * be unloaded because unloading is not safe in a multi-threaded + * application. * * The module is automatically looked up in the appropriate place (git or * installed directory). @@ -100,8 +99,7 @@ virDriverLoadModuleFunc(void *handle, */ int virDriverLoadModuleFull(const char *path, - const char *regfunc, - void **handle) + const char *regfunc) { void *rethandle = NULL; int (*regsym)(void); @@ -120,11 +118,7 @@ virDriverLoadModuleFull(const char *path, goto cleanup; } - if (handle) - VIR_STEAL_PTR(*handle, rethandle); - else - rethandle = NULL; - + rethandle = NULL; ret = 0; cleanup: @@ -136,12 +130,9 @@ virDriverLoadModuleFull(const char *path, #else /* ! HAVE_DLFCN_H */ int virDriverLoadModuleFull(const char *path ATTRIBUTE_UNUSED, - const char *regfunc ATTRIBUTE_UNUSED, - void **handle) + const char *regfunc ATTRIBUTE_UNUSED) { VIR_DEBUG("dlopen not available on this platform"); - if (handle) - *handle = NULL; return -1; } #endif /* ! HAVE_DLFCN_H */ @@ -164,7 +155,7 @@ virDriverLoadModule(const char *name, "LIBVIRT_DRIVER_DIR"))) return 1; - ret = virDriverLoadModuleFull(modfile, regfunc, NULL); + ret = virDriverLoadModuleFull(modfile, regfunc); VIR_FREE(modfile); diff --git a/src/driver.h b/src/driver.h index b071a3a782..e28c63ecc2 100644 --- a/src/driver.h +++ b/src/driver.h @@ -109,9 +109,8 @@ int virSetSharedStorageDriver(virStorageDriverPtr driver) ATTRIBUTE_RETURN_CHECK int virDriverLoadModule(const char *name, const char *regfunc); -int virDriverLoadModuleFull(const char *name, - const char *regfunc, - void **handle); +int virDriverLoadModuleFull(const char *path, + const char *regfunc); virConnectPtr virGetConnectInterface(void); virConnectPtr virGetConnectNetwork(void); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 053f4ecf26..aac2f53b01 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -97,7 +97,7 @@ virStorageDriverLoadBackendModule(const char *name, "LIBVIRT_STORAGE_BACKEND_DIR"))) return 1; - if ((ret = virDriverLoadModuleFull(modfile, regfunc, NULL)) != 0) { + if ((ret = virDriverLoadModuleFull(modfile, regfunc)) != 0) { if (forceload) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to load storage backend module '%s'"), -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
Now that we've activated two hacks to prevent unloading of modules, there is no point passing back a pointer to the loaded library handle.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 23 +++++++---------------- src/driver.h | 5 ++--- src/storage/storage_backend.c | 2 +- 3 files changed, 10 insertions(+), 20 deletions(-)
ACK Michal

The virFileFindResource method merely builds up the expected fully qualified path to the resource. It does not actually check if it exists on disk. The loadable module callers were mistakenly thinking a NULL indicates the file doesn't exist on disk, whereas it in fact indicates an out of memory error. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 2 +- src/storage/storage_backend.c | 2 +- src/util/virfile.c | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/driver.c b/src/driver.c index 4d314b3870..ddda1e71f7 100644 --- a/src/driver.c +++ b/src/driver.c @@ -153,7 +153,7 @@ virDriverLoadModule(const char *name, abs_topbuilddir "/src/.libs", DEFAULT_DRIVER_DIR, "LIBVIRT_DRIVER_DIR"))) - return 1; + return -1; ret = virDriverLoadModuleFull(modfile, regfunc); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index aac2f53b01..8c1dcf31b1 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -95,7 +95,7 @@ virStorageDriverLoadBackendModule(const char *name, abs_topbuilddir "/src/.libs", STORAGE_BACKEND_MODULE_DIR, "LIBVIRT_STORAGE_BACKEND_DIR"))) - return 1; + return -1; if ((ret = virDriverLoadModuleFull(modfile, regfunc)) != 0) { if (forceload) { diff --git a/src/util/virfile.c b/src/util/virfile.c index 5e9bd2007a..e12a584ca1 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1736,6 +1736,10 @@ static bool useDirOverride; * run from the source tree. Otherwise it will return the * path in the installed location. * + * Note that this function does not actually check whether + * the file exists on disk, it merely builds the fully + * qualified path where it is supposed to exist. + * * If @envname is non-NULL it will override all other * directory lookup. * -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
The virFileFindResource method merely builds up the expected fully qualified path to the resource. It does not actually check if it exists on disk. The loadable module callers were mistakenly thinking a NULL indicates the file doesn't exist on disk, whereas it in fact indicates an out of memory error.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 2 +- src/storage/storage_backend.c | 2 +- src/util/virfile.c | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-)
ACK Michal

Currently we do a access(R_OK) check to see whether a loadable module exists, treating failure as non-fatal. This is unreasonably loose, as a module which exists but has had incorrect permissions set will turn into a silent skip. We only want to skip loading if the module genuinely does not exist on disk, due to the optional package not being installed. Furthermore, checking the return value of virDriverLoadModuleFile() is not a suitable witness that the module does not exist. This method can return NULL if dlopen() fails, for example due to being unable to resolve symbols in the library. This is should always be reported as an error because it is a sign of the bad installation where either the module build doesn't match the libvirtd build, or where some 3rd party libraries are missing or broken. Both these problems can be fixed by using virFileExists in the caller instead. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/driver.c b/src/driver.c index ddda1e71f7..e02efe2615 100644 --- a/src/driver.c +++ b/src/driver.c @@ -53,11 +53,6 @@ virDriverLoadModuleFile(const char *file) VIR_DEBUG("Load module file '%s'", file); - if (access(file, R_OK) < 0) { - VIR_INFO("Module %s not accessible", file); - return NULL; - } - virUpdateSelfLastChanged(file); if (!(handle = dlopen(file, flags))) @@ -105,11 +100,14 @@ virDriverLoadModuleFull(const char *path, int (*regsym)(void); int ret = -1; - if (!(rethandle = virDriverLoadModuleFile(path))) { - ret = 1; - goto cleanup; + if (!virFileExists(path)) { + VIR_INFO("Module '%s' does not exists", path); + return 1; } + if (!(rethandle = virDriverLoadModuleFile(path))) + goto cleanup; + if (!(regsym = virDriverLoadModuleFunc(rethandle, regfunc))) goto cleanup; -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
Currently we do a access(R_OK) check to see whether a loadable module exists, treating failure as non-fatal. This is unreasonably loose, as a module which exists but has had incorrect permissions set will turn into a silent skip. We only want to skip loading if the module genuinely does not exist on disk, due to the optional package not being installed.
Furthermore, checking the return value of virDriverLoadModuleFile() is not a suitable witness that the module does not exist. This method can return NULL if dlopen() fails, for example due to being unable to resolve symbols in the library. This is should always be reported as an error because it is a sign of the bad installation where either the module build doesn't match the libvirtd build, or where some 3rd party libraries are missing or broken.
Both these problems can be fixed by using virFileExists in the caller instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
ACK Michal

The driver module loading code is one of the few places that still uses VIR_ERROR for reporting failures. Convert it to normal error reporting APIs. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/src/driver.c b/src/driver.c index e02efe2615..9b137c39e4 100644 --- a/src/driver.c +++ b/src/driver.c @@ -33,6 +33,7 @@ VIR_LOG_INIT("driver"); +#define VIR_FROM_THIS VIR_FROM_NONE /* XXX re-implement this for other OS, or use libtools helper lib ? */ #define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/connection-driver" @@ -55,8 +56,11 @@ virDriverLoadModuleFile(const char *file) virUpdateSelfLastChanged(file); - if (!(handle = dlopen(file, flags))) - VIR_ERROR(_("failed to load module %s %s"), file, dlerror()); + if (!(handle = dlopen(file, flags))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load module '%s': %s"), file, dlerror()); + return NULL; + } return handle; } @@ -64,14 +68,19 @@ virDriverLoadModuleFile(const char *file) static void * virDriverLoadModuleFunc(void *handle, + const char *file, const char *funcname) { void *regsym; VIR_DEBUG("Lookup function '%s'", funcname); - if (!(regsym = dlsym(handle, funcname))) - VIR_ERROR(_("Missing module registration symbol %s"), funcname); + if (!(regsym = dlsym(handle, funcname))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to find symbol '%s' in module '%s': %s"), + funcname, file, dlerror()); + return NULL; + } return regsym; } @@ -108,11 +117,18 @@ virDriverLoadModuleFull(const char *path, if (!(rethandle = virDriverLoadModuleFile(path))) goto cleanup; - if (!(regsym = virDriverLoadModuleFunc(rethandle, regfunc))) + if (!(regsym = virDriverLoadModuleFunc(rethandle, path, regfunc))) goto cleanup; if ((*regsym)() < 0) { - VIR_ERROR(_("Failed module registration %s"), regfunc); + /* regsym() should report an error itself, but lets + * just make sure */ + virErrorPtr err = virGetLastError(); + if (err == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to execute symbol '%s' in module '%s'"), + regfunc, path); + } goto cleanup; } @@ -131,7 +147,11 @@ virDriverLoadModuleFull(const char *path ATTRIBUTE_UNUSED, const char *regfunc ATTRIBUTE_UNUSED) { VIR_DEBUG("dlopen not available on this platform"); - return -1; + /* Since we have no dlopen(), but definition we have no + * loadable modules on disk, so we can resaonably + * return '1' instead of an error. + */ + return 1; } #endif /* ! HAVE_DLFCN_H */ -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
The driver module loading code is one of the few places that still uses VIR_ERROR for reporting failures. Convert it to normal error reporting APIs.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/src/driver.c b/src/driver.c index e02efe2615..9b137c39e4 100644 --- a/src/driver.c +++ b/src/driver.c @@ -33,6 +33,7 @@
VIR_LOG_INIT("driver");
+#define VIR_FROM_THIS VIR_FROM_NONE
/* XXX re-implement this for other OS, or use libtools helper lib ? */ #define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/connection-driver" @@ -55,8 +56,11 @@ virDriverLoadModuleFile(const char *file)
virUpdateSelfLastChanged(file);
- if (!(handle = dlopen(file, flags))) - VIR_ERROR(_("failed to load module %s %s"), file, dlerror()); + if (!(handle = dlopen(file, flags))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load module '%s': %s"), file, dlerror());
Sweet. dlerror() returns 'char *' instead of 'const char *' even though it's returning a pointer to statically allocated buffer.
+ return NULL; + }
return handle; }
ACK Michal

Currently the driver module loading code does not report an error if the driver module is physically missing on disk. This is useful for distro packaging optional pieces. When the daemons are split up into one daemon per driver, we will expect module loading to always succeed. If a driver is not desired, the entire daemon should not be installed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 37 ++++++++++++++++++++++++++----------- src/driver.h | 6 ++++-- src/remote/remote_daemon.c | 2 +- src/storage/storage_backend.c | 9 +-------- tests/virdrivermoduletest.c | 2 +- 5 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/driver.c b/src/driver.c index 9b137c39e4..447f61d554 100644 --- a/src/driver.c +++ b/src/driver.c @@ -103,15 +103,22 @@ virDriverLoadModuleFunc(void *handle, */ int virDriverLoadModuleFull(const char *path, - const char *regfunc) + const char *regfunc, + bool required) { void *rethandle = NULL; int (*regsym)(void); int ret = -1; if (!virFileExists(path)) { - VIR_INFO("Module '%s' does not exists", path); - return 1; + if (required) { + virReportSystemError(errno, + _("Failed to find module '%s'"), path); + return -1; + } else { + VIR_INFO("Module '%s' does not exist", path); + return 1; + } } if (!(rethandle = virDriverLoadModuleFile(path))) @@ -144,21 +151,29 @@ virDriverLoadModuleFull(const char *path, #else /* ! HAVE_DLFCN_H */ int virDriverLoadModuleFull(const char *path ATTRIBUTE_UNUSED, - const char *regfunc ATTRIBUTE_UNUSED) + const char *regfunc ATTRIBUTE_UNUSED, + bool required) { VIR_DEBUG("dlopen not available on this platform"); - /* Since we have no dlopen(), but definition we have no - * loadable modules on disk, so we can resaonably - * return '1' instead of an error. - */ - return 1; + if (required) { + virReportSystemError(ENOSYS, + _("Failed to find module '%s': %s"), path); + return -1; + } else { + /* Since we have no dlopen(), but definition we have no + * loadable modules on disk, so we can resaonably + * return '1' instead of an error. + */ + return 1; + } } #endif /* ! HAVE_DLFCN_H */ int virDriverLoadModule(const char *name, - const char *regfunc) + const char *regfunc, + bool required) { char *modfile = NULL; int ret; @@ -173,7 +188,7 @@ virDriverLoadModule(const char *name, "LIBVIRT_DRIVER_DIR"))) return -1; - ret = virDriverLoadModuleFull(modfile, regfunc); + ret = virDriverLoadModuleFull(modfile, regfunc, required); VIR_FREE(modfile); diff --git a/src/driver.h b/src/driver.h index e28c63ecc2..b4e50ab987 100644 --- a/src/driver.h +++ b/src/driver.h @@ -108,9 +108,11 @@ int virSetSharedSecretDriver(virSecretDriverPtr driver) ATTRIBUTE_RETURN_CHECK; int virSetSharedStorageDriver(virStorageDriverPtr driver) ATTRIBUTE_RETURN_CHECK; int virDriverLoadModule(const char *name, - const char *regfunc); + const char *regfunc, + bool required); int virDriverLoadModuleFull(const char *path, - const char *regfunc); + const char *regfunc, + bool required); virConnectPtr virGetConnectInterface(void); virConnectPtr virGetConnectNetwork(void); diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 3e02297eee..b4f89d4fd7 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -295,7 +295,7 @@ static int daemonErrorLogFilter(virErrorPtr err, int priority) #define VIR_DAEMON_LOAD_MODULE(func, module) \ - virDriverLoadModule(module, #func) + virDriverLoadModule(module, #func, false) static void daemonInitialize(void) { /* diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 8c1dcf31b1..cb1bcc0944 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -97,14 +97,7 @@ virStorageDriverLoadBackendModule(const char *name, "LIBVIRT_STORAGE_BACKEND_DIR"))) return -1; - if ((ret = virDriverLoadModuleFull(modfile, regfunc)) != 0) { - if (forceload) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to load storage backend module '%s'"), - name); - ret = -1; - } - } + ret = virDriverLoadModuleFull(modfile, regfunc, forceload); VIR_FREE(modfile); diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c index 6c63ecd52b..125183327b 100644 --- a/tests/virdrivermoduletest.c +++ b/tests/virdrivermoduletest.c @@ -41,7 +41,7 @@ static int testDriverModule(const void *args) const struct testDriverModuleData *data = args; /* coverity[leaked_storage] */ - if (virDriverLoadModule(data->module, data->regfunc) != 0) + if (virDriverLoadModule(data->module, data->regfunc, true) != 0) return -1; return 0; -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
Currently the driver module loading code does not report an error if the driver module is physically missing on disk. This is useful for distro packaging optional pieces. When the daemons are split up into one daemon per driver, we will expect module loading to always succeed. If a driver is not desired, the entire daemon should not be installed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 37 ++++++++++++++++++++++++++----------- src/driver.h | 6 ++++-- src/remote/remote_daemon.c | 2 +- src/storage/storage_backend.c | 9 +-------- tests/virdrivermoduletest.c | 2 +- 5 files changed, 33 insertions(+), 23 deletions(-)
ACK with this squashed in: diff --git i/src/security/virt-aa-helper.c w/src/security/virt-aa-helper.c index a1bc1090bf..ee5e3b0701 100644 --- i/src/security/virt-aa-helper.c +++ w/src/security/virt-aa-helper.c @@ -964,7 +964,7 @@ get_files(vahControl * ctl) /* load the storage driver so that backing store can be accessed */ #ifdef WITH_STORAGE - virDriverLoadModule("storage", "storageRegister"); + virDriverLoadModule("storage", "storageRegister", false); #endif for (i = 0; i < ctl->def->ndisks; i++) { Michal

The libvirtd daemon currently ignores the return status of virDriverLoadModule entirely. This is way too loose, resulting in many important problems going undiagnosed, resulting in a libvirtd that may never work correctly. We should only ignore a non-existant module, and pass back any fatal errors. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 61 +++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index b4f89d4fd7..27377fe3bc 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -87,6 +87,7 @@ enum { VIR_DAEMON_ERR_CONFIG, VIR_DAEMON_ERR_HOOKS, VIR_DAEMON_ERR_AUDIT, + VIR_DAEMON_ERR_DRIVER, VIR_DAEMON_ERR_LAST }; @@ -102,7 +103,8 @@ VIR_ENUM_IMPL(virDaemonErr, VIR_DAEMON_ERR_LAST, "Unable to initialize network sockets", "Unable to load configuration file", "Unable to look for hook scripts", - "Unable to initialize audit system") + "Unable to initialize audit system", + "Unable to initialize driver") static int daemonForkIntoBackground(const char *argv0) { @@ -294,9 +296,7 @@ static int daemonErrorLogFilter(virErrorPtr err, int priority) } -#define VIR_DAEMON_LOAD_MODULE(func, module) \ - virDriverLoadModule(module, #func, false) -static void daemonInitialize(void) +static int daemonInitialize(void) { /* * Note that the order is important: the first ones have a higher @@ -305,52 +305,60 @@ static void daemonInitialize(void) * driver, since their resources must be auto-started before any * domains can be auto-started. */ - /* We don't care if any of these fail, because the whole point - * is to allow users to only install modules they want to use. - * 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_NETWORK - VIR_DAEMON_LOAD_MODULE(networkRegister, "network"); + if (virDriverLoadModule("network", "networkRegister", false) < 0) + return -1; #endif #ifdef WITH_INTERFACE - VIR_DAEMON_LOAD_MODULE(interfaceRegister, "interface"); + if (virDriverLoadModule("interface", "interfaceRegister", false) < 0) + return -1; #endif #ifdef WITH_STORAGE - VIR_DAEMON_LOAD_MODULE(storageRegister, "storage"); + if (virDriverLoadModule("storage", "storageRegister", false) < 0) + return -1; #endif #ifdef WITH_NODE_DEVICES - VIR_DAEMON_LOAD_MODULE(nodedevRegister, "nodedev"); + if (virDriverLoadModule("nodedev", "nodedevRegister", false) < 0) + return -1; #endif #ifdef WITH_SECRETS - VIR_DAEMON_LOAD_MODULE(secretRegister, "secret"); + if (virDriverLoadModule("secret", "secretRegister", false) < 0) + return -1; #endif #ifdef WITH_NWFILTER - VIR_DAEMON_LOAD_MODULE(nwfilterRegister, "nwfilter"); + if (virDriverLoadModule("nwfilter", "nwfilterRegister", false) < 0) + return -1; #endif #ifdef WITH_LIBXL - VIR_DAEMON_LOAD_MODULE(libxlRegister, "libxl"); + if (virDriverLoadModule("libxl", "libxlRegister", false) < 0) + return -1; #endif #ifdef WITH_QEMU - VIR_DAEMON_LOAD_MODULE(qemuRegister, "qemu"); + if (virDriverLoadModule("qemu", "qemuRegister", false) < 0) + return -1; #endif #ifdef WITH_LXC - VIR_DAEMON_LOAD_MODULE(lxcRegister, "lxc"); + if (virDriverLoadModule("lxc", "lxcRegister", false) < 0) + return -1; #endif #ifdef WITH_UML - VIR_DAEMON_LOAD_MODULE(umlRegister, "uml"); + if (virDriverLoadModule("uml", "umlRegister", false) < 0) + return -1; #endif #ifdef WITH_VBOX - VIR_DAEMON_LOAD_MODULE(vboxRegister, "vbox"); + if (virDriverLoadModule("vbox", "vboxRegister", false) < 0) + return -1; #endif #ifdef WITH_BHYVE - VIR_DAEMON_LOAD_MODULE(bhyveRegister, "bhyve"); + if (virDriverLoadModule("bhyve", "bhyveRegister", false) < 0) + return -1; #endif #ifdef WITH_VZ - VIR_DAEMON_LOAD_MODULE(vzRegister, "vz"); + if (virDriverLoadModule("vz", "vzRegister", false) < 0) + return -1; #endif + return 0; } -#undef VIR_DAEMON_LOAD_MODULE static int ATTRIBUTE_NONNULL(3) @@ -1283,7 +1291,7 @@ int main(int argc, char **argv) { } if (!(dmn = virNetDaemonNew())) { - ret = VIR_DAEMON_ERR_INIT; + ret = VIR_DAEMON_ERR_DRIVER; goto cleanup; } @@ -1309,7 +1317,10 @@ int main(int argc, char **argv) { goto cleanup; } - daemonInitialize(); + if (daemonInitialize() < 0) { + ret = VIR_DAEMON_ERR_INIT; + goto cleanup; + } remoteProcs[REMOTE_PROC_AUTH_LIST].needAuth = false; remoteProcs[REMOTE_PROC_AUTH_SASL_INIT].needAuth = false; -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
The libvirtd daemon currently ignores the return status of virDriverLoadModule entirely. This is way too loose, resulting in many important problems going undiagnosed, resulting in a libvirtd that may never work correctly. We should only ignore a non-existant module, and pass back any fatal errors.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_daemon.c | 61 +++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 25 deletions(-)
ACK Michal

Currently the remote driver extracts the transport from URI scheme and plays games to temporarily hide the driver part when formatting URIs. Refactor the code to split the URI scheme upfront so the two pieces are easily available where needed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 73 ++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d3b588c374..436324d252 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -24,7 +24,6 @@ #include <config.h> #include <unistd.h> -#include <assert.h> #include "virnetclient.h" #include "virnetclientprogram.h" @@ -162,7 +161,26 @@ static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho /*----------------------------------------------------------------------*/ /* Helper functions for remoteOpen. */ -static char *get_transport_from_scheme(char *scheme); +static int remoteSplitURIScheme(virURIPtr uri, + char **driver, + char **transport) +{ + char *p = strchr(uri->scheme, '+'); + + *driver = *transport = NULL; + + if (VIR_STRNDUP(*driver, uri->scheme, p ? p - uri->scheme : -1) < 0) + return -1; + + if (p && + VIR_STRDUP(*transport, p + 1) < 0) { + VIR_FREE(*driver); + return -1; + } + + return 0; +} + static int remoteStateInitialize(bool privileged ATTRIBUTE_UNUSED, @@ -715,11 +733,12 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn, static int doRemoteOpen(virConnectPtr conn, struct private_data *priv, + const char *driver_str, + const char *transport_str, virConnectAuthPtr auth ATTRIBUTE_UNUSED, virConfPtr conf, unsigned int flags) { - char *transport_str = NULL; enum { trans_tls, trans_unix, @@ -738,8 +757,6 @@ doRemoteOpen(virConnectPtr conn, * URIs we don't care about */ if (conn->uri) { - transport_str = get_transport_from_scheme(conn->uri->scheme); - if (!transport_str) { if (conn->uri->server) transport = trans_tls; @@ -873,26 +890,16 @@ doRemoteOpen(virConnectPtr conn, goto failed; } else { virURI tmpuri = { - .scheme = conn->uri->scheme, + .scheme = (char *)driver_str, .query = virURIFormatParams(conn->uri), .path = conn->uri->path, .fragment = conn->uri->fragment, }; - /* Evil, blank out transport scheme temporarily */ - if (transport_str) { - assert(transport_str[-1] == '+'); - transport_str[-1] = '\0'; - } - name = virURIFormat(&tmpuri); VIR_FREE(tmpuri.query); - /* Restore transport scheme */ - if (transport_str) - transport_str[-1] = '+'; - if (!name) goto failed; } @@ -1297,14 +1304,23 @@ remoteConnectOpen(virConnectPtr conn, unsigned int flags) { struct private_data *priv; - int ret, rflags = 0; + int ret = VIR_DRV_OPEN_ERROR; + int rflags = 0; const char *autostart = virGetEnvBlockSUID("LIBVIRT_AUTOSTART"); + char *driver = NULL; + char *transport = NULL; - if (inside_daemon && (!conn->uri || !conn->uri->server)) - return VIR_DRV_OPEN_DECLINED; + if (conn->uri && + remoteSplitURIScheme(conn->uri, &driver, &transport) < 0) + goto cleanup; + + if (inside_daemon && (!conn->uri || !conn->uri->server)) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } if (!(priv = remoteAllocPrivateData())) - return VIR_DRV_OPEN_ERROR; + goto cleanup; if (flags & VIR_CONNECT_RO) rflags |= VIR_DRV_OPEN_REMOTE_RO; @@ -1319,8 +1335,7 @@ remoteConnectOpen(virConnectPtr conn, !conn->uri->server && conn->uri->path && conn->uri->scheme && - ((strchr(conn->uri->scheme, '+') == 0)|| - (strstr(conn->uri->scheme, "+unix") != NULL)) && + (transport == NULL || STREQ(transport, "unix")) && (STREQ(conn->uri->path, "/session") || STRPREFIX(conn->uri->scheme, "test+")) && geteuid() > 0) { @@ -1348,7 +1363,7 @@ remoteConnectOpen(virConnectPtr conn, } } - ret = doRemoteOpen(conn, priv, auth, conf, rflags); + ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags); if (ret != VIR_DRV_OPEN_SUCCESS) { conn->privateData = NULL; remoteDriverUnlock(priv); @@ -1357,18 +1372,14 @@ remoteConnectOpen(virConnectPtr conn, conn->privateData = priv; remoteDriverUnlock(priv); } + + cleanup: + VIR_FREE(driver); + VIR_FREE(transport); return ret; } -/* In a string "driver+transport" return a pointer to "transport". */ -static char * -get_transport_from_scheme(char *scheme) -{ - char *p = strchr(scheme, '+'); - return p ? p + 1 : NULL; -} - /*----------------------------------------------------------------------*/ -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
Currently the remote driver extracts the transport from URI scheme and plays games to temporarily hide the driver part when formatting URIs. Refactor the code to split the URI scheme upfront so the two pieces are easily available where needed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 73 ++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 31 deletions(-)
ACK Michal

The code for building UNIX socket paths will be getting more complex to cope with accessing various different daemons. Refactor it to eliminate the code duplication and isolation the logic for constructing paths. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 69 ++++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 436324d252..b4b034423a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -710,6 +710,39 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn, var->ignore = 1; \ continue; \ } + + +static char *remoteGetUNIXSocketNonRoot(void) +{ + char *sockname = NULL; + char *userdir = virGetUserRuntimeDirectory(); + + if (!userdir) + return NULL; + + if (virAsprintf(&sockname, "%s/" LIBVIRTD_USER_UNIX_SOCKET, userdir) < 0) { + VIR_FREE(userdir); + return NULL; + } + VIR_FREE(userdir); + + VIR_DEBUG("Chosen UNIX sockname %s", sockname); + return sockname; +} + +static char *remoteGetUNIXSocketRoot(unsigned int flags) +{ + char *sockname = NULL; + + if (VIR_STRDUP(sockname, + flags & VIR_DRV_OPEN_REMOTE_RO ? + LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0) + return NULL; + + VIR_DEBUG("Chosen UNIX sockname %s", sockname); + return sockname; +} + /* * URIs that this driver needs to handle: * @@ -971,9 +1004,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (VIR_STRDUP(sockname, - flags & VIR_DRV_OPEN_REMOTE_RO ? - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0) + if (!(sockname = remoteGetUNIXSocketRoot(flags))) goto failed; } @@ -1008,9 +1039,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (VIR_STRDUP(sockname, - flags & VIR_DRV_OPEN_REMOTE_RO ? - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0) + if (!(sockname = remoteGetUNIXSocketRoot(flags))) goto failed; } @@ -1037,24 +1066,12 @@ doRemoteOpen(virConnectPtr conn, #ifndef WIN32 case trans_unix: if (!sockname) { - if (flags & VIR_DRV_OPEN_REMOTE_USER) { - char *userdir = virGetUserRuntimeDirectory(); - - if (!userdir) - goto failed; - - if (virAsprintf(&sockname, "%s/" LIBVIRTD_USER_UNIX_SOCKET, userdir) < 0) { - VIR_FREE(userdir); - goto failed; - } - VIR_FREE(userdir); - } else { - if (VIR_STRDUP(sockname, - flags & VIR_DRV_OPEN_REMOTE_RO ? - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0) - goto failed; - } - VIR_DEBUG("Proceeding with sockname %s", sockname); + if (flags & VIR_DRV_OPEN_REMOTE_USER) + sockname = remoteGetUNIXSocketNonRoot(); + else + sockname = remoteGetUNIXSocketRoot(flags); + if (!sockname) + goto failed; } if ((flags & VIR_DRV_OPEN_REMOTE_AUTOSTART) && @@ -1087,9 +1104,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (VIR_STRDUP(sockname, - flags & VIR_DRV_OPEN_REMOTE_RO ? - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0) + if (!(sockname = remoteGetUNIXSocketRoot(flags))) goto failed; } -- 2.14.3

On 04/19/2018 07:09 PM, Daniel P. Berrangé wrote:
The code for building UNIX socket paths will be getting more complex to cope with accessing various different daemons. Refactor it to eliminate the code duplication and isolation the logic for constructing paths.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 69 ++++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 27 deletions(-)
ACK Up until and including this patch, you can merge these as they fix real problems / improve things. Michal

Prepare for reusing libvirtd source to create other daemons by making various bits conditionalized. This will avoid cut+paste of huge amount of source code for each new daemon. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/Makefile.inc.am | 21 +++++ src/remote/remote_daemon.c | 160 +++++++++++++++++++++++--------------- src/remote/remote_daemon_config.c | 36 +++++++-- src/remote/remote_daemon_config.h | 9 ++- src/remote/remote_driver.h | 1 - 5 files changed, 153 insertions(+), 74 deletions(-) diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index 12600b8bb5..a6c60a35b3 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -140,6 +140,27 @@ libvirtd_CFLAGS = \ -I$(srcdir)/access \ -I$(srcdir)/conf \ -I$(srcdir)/rpc \ + -DSOCK_NAME="\"libvirt-sock\"" \ + -DSOCK_NAME_RO="\"libvirt-sock-ro\"" \ + -DSOCK_NAME_ADMIN="\"libvirt-admin-sock\"" \ + -DAPP_NAME="\"libvirtd\"" \ + -DLOAD_NETWORK \ + -DLOAD_INTERFACE \ + -DLOAD_STORAGE \ + -DLOAD_NODE_DEVICES \ + -DLOAD_SECRETS \ + -DLOAD_NWFILTER \ + -DLOAD_XEN \ + -DLOAD_LIBXL \ + -DLOAD_QEMU \ + -DLOAD_LXC \ + -DLOAD_UML \ + -DLOAD_VBOX \ + -DLOAD_BHYVE \ + -DLOAD_VZ \ + -DCONFIG_MIGRATE \ + -DWITH_NET_IP \ + -DREQUIRE_MODULE=false \ $(NULL) libvirtd_LDFLAGS = \ diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index 27377fe3bc..0f53b7989d 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -221,19 +221,19 @@ daemonUnixSocketPaths(struct daemonConfig *config, char *rundir = NULL; if (config->unix_sock_dir) { - if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir) < 0) + if (virAsprintf(sockfile, "%s/" SOCK_NAME, config->unix_sock_dir) < 0) goto cleanup; if (privileged) { - if (virAsprintf(rosockfile, "%s/libvirt-sock-ro", config->unix_sock_dir) < 0 || - virAsprintf(admsockfile, "%s/libvirt-admin-sock", config->unix_sock_dir) < 0) + if (virAsprintf(rosockfile, "%s/" SOCK_NAME_RO, config->unix_sock_dir) < 0 || + virAsprintf(admsockfile, "%s/" SOCK_NAME_ADMIN, config->unix_sock_dir) < 0) goto cleanup; } } else { if (privileged) { - if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock") < 0 || - VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro") < 0 || - VIR_STRDUP(*admsockfile, LOCALSTATEDIR "/run/libvirt/libvirt-admin-sock") < 0) + if (VIR_STRDUP(*sockfile, LOCALSTATEDIR "/run/libvirt/" SOCK_NAME) < 0 || + VIR_STRDUP(*rosockfile, LOCALSTATEDIR "/run/libvirt/" SOCK_NAME_RO) < 0 || + VIR_STRDUP(*admsockfile, LOCALSTATEDIR "/run/libvirt/" SOCK_NAME_ADMIN) < 0) goto cleanup; } else { mode_t old_umask; @@ -248,8 +248,8 @@ daemonUnixSocketPaths(struct daemonConfig *config, } umask(old_umask); - if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 || - virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) < 0) + if (virAsprintf(sockfile, "%s/" SOCK_NAME, rundir) < 0 || + virAsprintf(admsockfile, "%s/" SOCK_NAME_ADMIN, rundir) < 0) goto cleanup; } } @@ -305,56 +305,56 @@ static int daemonInitialize(void) * driver, since their resources must be auto-started before any * domains can be auto-started. */ -#ifdef WITH_NETWORK - if (virDriverLoadModule("network", "networkRegister", false) < 0) +#if defined(WITH_NETWORK) && defined(LOAD_NETWORK) + if (virDriverLoadModule("network", "networkRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_INTERFACE - if (virDriverLoadModule("interface", "interfaceRegister", false) < 0) +#if defined(WITH_INTERFACE) && defined(LOAD_INTERFACE) + if (virDriverLoadModule("interface", "interfaceRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_STORAGE - if (virDriverLoadModule("storage", "storageRegister", false) < 0) +#if defined(WITH_STORAGE) && defined(LOAD_STORAGE) + if (virDriverLoadModule("storage", "storageRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_NODE_DEVICES - if (virDriverLoadModule("nodedev", "nodedevRegister", false) < 0) +#if defined(WITH_NODE_DEVICES) && defined(LOAD_NODE_DEVICES) + if (virDriverLoadModule("nodedev", "nodedevRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_SECRETS - if (virDriverLoadModule("secret", "secretRegister", false) < 0) +#if defined(WITH_SECRETS) && defined(LOAD_SECRETS) + if (virDriverLoadModule("secret", "secretRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_NWFILTER - if (virDriverLoadModule("nwfilter", "nwfilterRegister", false) < 0) +#if defined(WITH_NWFILTER) && defined(LOAD_NWFILTER) + if (virDriverLoadModule("nwfilter", "nwfilterRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_LIBXL - if (virDriverLoadModule("libxl", "libxlRegister", false) < 0) +#if defined(WITH_LIBXL) && defined(LOAD_LIBXL) + if (virDriverLoadModule("libxl", "libxlRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_QEMU - if (virDriverLoadModule("qemu", "qemuRegister", false) < 0) +#if defined(WITH_QEMU) && defined(LOAD_QEMU) + if (virDriverLoadModule("qemu", "qemuRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_LXC - if (virDriverLoadModule("lxc", "lxcRegister", false) < 0) +#if defined(WITH_LXC) && defined(LOAD_LXC) + if (virDriverLoadModule("lxc", "lxcRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_UML - if (virDriverLoadModule("uml", "umlRegister", false) < 0) +#if defined(WITH_UML) && defined(LOAD_UML) + if (virDriverLoadModule("uml", "umlRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_VBOX - if (virDriverLoadModule("vbox", "vboxRegister", false) < 0) +#if defined(WITH_VBOX) && defined(LOAD_VBOX) + if (virDriverLoadModule("vbox", "vboxRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_BHYVE - if (virDriverLoadModule("bhyve", "bhyveRegister", false) < 0) +#if defined(WITH_BHYVE) && defined(LOAD_BHYVE) + if (virDriverLoadModule("bhyve", "bhyveRegister", REQUIRE_MODULE) < 0) return -1; #endif -#ifdef WITH_VZ - if (virDriverLoadModule("vz", "vzRegister", false) < 0) +#if defined(WITH_VZ) && defined(LOAD_VZ) + if (virDriverLoadModule("vz", "vzRegister", REQUIRE_MODULE) < 0) return -1; #endif return 0; @@ -368,15 +368,19 @@ daemonSetupNetworking(virNetServerPtr srv, const char *sock_path, const char *sock_path_ro, const char *sock_path_adm, +#ifdef WITH_NET_IP bool ipsock, - bool privileged) +#endif + bool privileged ATTRIBUTE_UNUSED) { virNetServerServicePtr svc = NULL; virNetServerServicePtr svcAdm = NULL; virNetServerServicePtr svcRO = NULL; +#ifdef WITH_NET_IP virNetServerServicePtr svcTCP = NULL; -#if WITH_GNUTLS +# if WITH_GNUTLS virNetServerServicePtr svcTLS = NULL; +# endif #endif gid_t unix_sock_gid = 0; int unix_sock_ro_mask = 0; @@ -440,8 +444,10 @@ daemonSetupNetworking(virNetServerPtr srv, } if (virNetServerAddService(srv, svc, +#ifdef WITH_NET_IP config->mdns_adv && !ipsock ? "_libvirt._tcp" : +#endif NULL) < 0) goto cleanup; @@ -467,6 +473,7 @@ daemonSetupNetworking(virNetServerPtr srv, goto cleanup; } +#ifdef WITH_NET_IP if (ipsock) { if (config->listen_tcp) { VIR_DEBUG("Registering TCP socket %s:%s", @@ -475,9 +482,9 @@ daemonSetupNetworking(virNetServerPtr srv, config->tcp_port, AF_UNSPEC, config->auth_tcp, -#if WITH_GNUTLS +# if WITH_GNUTLS NULL, -#endif +# endif false, config->max_queued_clients, config->max_client_requests))) @@ -488,7 +495,7 @@ daemonSetupNetworking(virNetServerPtr srv, goto cleanup; } -#if WITH_GNUTLS +# if WITH_GNUTLS if (config->listen_tls) { virNetTLSContextPtr ctxt = NULL; @@ -552,23 +559,26 @@ daemonSetupNetworking(virNetServerPtr srv, virObjectUnref(ctxt); } -#else +# else (void)privileged; if (config->listen_tls) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("This libvirtd build does not support TLS")); goto cleanup; } -#endif +# endif } +#endif #if WITH_SASL - if (config->auth_unix_rw == REMOTE_AUTH_SASL || - (sock_path_ro && config->auth_unix_ro == REMOTE_AUTH_SASL) || -# if WITH_GNUTLS - (ipsock && config->listen_tls && config->auth_tls == REMOTE_AUTH_SASL) || + if (config->auth_unix_rw == REMOTE_AUTH_SASL +# ifdef WITH_NET_IP +# if WITH_GNUTLS + || (ipsock && config->listen_tls && config->auth_tls == REMOTE_AUTH_SASL) +# endif + || (ipsock && config->listen_tcp && config->auth_tcp == REMOTE_AUTH_SASL) # endif - (ipsock && config->listen_tcp && config->auth_tcp == REMOTE_AUTH_SASL)) { + || (sock_path_ro && config->auth_unix_ro == REMOTE_AUTH_SASL)) { saslCtxt = virNetSASLContextNewServer( (const char *const*)config->sasl_allowed_username_list); if (!saslCtxt) @@ -579,10 +589,12 @@ daemonSetupNetworking(virNetServerPtr srv, ret = 0; cleanup: -#if WITH_GNUTLS +#ifdef WITH_NET_IP +# if WITH_GNUTLS virObjectUnref(svcTLS); -#endif +# endif virObjectUnref(svcTCP); +#endif virObjectUnref(svcRO); virObjectUnref(svcAdm); virObjectUnref(svc); @@ -643,7 +655,7 @@ daemonSetupLogging(struct daemonConfig *config, /* Define the default output. This is only applied if there was no setting * from either the config or the environment. */ - if (virLogSetDefaultOutput("libvirtd.log", godaemon, privileged) < 0) + if (virLogSetDefaultOutput(APP_NAME ".log", godaemon, privileged) < 0) return -1; if (virLogGetNbOutputs() == 0) @@ -872,6 +884,7 @@ static int daemonStateInit(virNetDaemonPtr dmn) return 0; } +#ifdef CONFIG_MIGRATE static int migrateProfile(void) { char *old_base = NULL; @@ -944,6 +957,7 @@ static int migrateProfile(void) return ret; } +#endif static int daemonSetupHostUUID(const struct daemonConfig *config) @@ -1006,46 +1020,55 @@ daemonUsage(const char *argv0, bool privileged) " Default paths:\n" "\n" " Configuration file (unless overridden by -f):\n" - " %s\n" + " %s.conf\n" "\n" " Sockets:\n" " %s\n" " %s\n" "\n" +#ifdef WITH_NET_IP " TLS:\n" " CA certificate: %s\n" " Server certificate: %s\n" " Server private key: %s\n" +#endif "\n" " PID file (unless overridden by -p):\n" - " %s/run/libvirtd.pid\n" + " %s/run/%s.pid\n" "\n"), - LIBVIRTD_CONFIGURATION_FILE, - LIBVIRTD_PRIV_UNIX_SOCKET, - LIBVIRTD_PRIV_UNIX_SOCKET_RO, + SYSCONFDIR "/libvirt/" APP_NAME, + LOCALSTATEDIR "/run/libvirt/" SOCK_NAME, + LOCALSTATEDIR "/run/libvirt/" SOCK_NAME_RO, +#ifdef WITH_NET_IP LIBVIRT_CACERT, LIBVIRT_SERVERCERT, LIBVIRT_SERVERKEY, - LOCALSTATEDIR); +#endif + LOCALSTATEDIR, APP_NAME); } else { - fprintf(stderr, "%s", + fprintf(stderr, _("\n" " Default paths:\n" "\n" " Configuration file (unless overridden by -f):\n" - " $XDG_CONFIG_HOME/libvirt/libvirtd.conf\n" + " $XDG_CONFIG_HOME/libvirt/%s.conf\n" "\n" " Sockets:\n" - " $XDG_RUNTIME_DIR/libvirt/libvirt-sock\n" + " $XDG_RUNTIME_DIR/libvirt/%s\n" "\n" +#ifdef WITH_NET_IP " TLS:\n" " CA certificate: $HOME/.pki/libvirt/cacert.pem\n" " Server certificate: $HOME/.pki/libvirt/servercert.pem\n" " Server private key: $HOME/.pki/libvirt/serverkey.pem\n" +#endif "\n" " PID file:\n" - " $XDG_RUNTIME_DIR/libvirt/libvirtd.pid\n" - "\n")); + " $XDG_RUNTIME_DIR/libvirt/%s.pid\n" + "\n"), + APP_NAME, + SOCK_NAME, + APP_NAME); } } @@ -1189,11 +1212,13 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } +#ifdef CONFIG_MIGRATE if (!privileged && migrateProfile() < 0) { VIR_ERROR(_("Exiting due to failure to migrate profile")); exit(EXIT_FAILURE); } +#endif if (daemonSetupHostUUID(config) < 0) { VIR_ERROR(_("Can't setup host uuid")); @@ -1215,7 +1240,7 @@ int main(int argc, char **argv) { if (!pid_file && virPidFileConstructPath(privileged, LOCALSTATEDIR, - "libvirtd", + APP_NAME, &pid_file) < 0) { VIR_ERROR(_("Can't determine pid file path.")); exit(EXIT_FAILURE); @@ -1295,7 +1320,7 @@ int main(int argc, char **argv) { goto cleanup; } - if (!(srv = virNetServerNew("libvirtd", 1, + if (!(srv = virNetServerNew(APP_NAME, 1, config->min_workers, config->max_workers, config->prio_workers, @@ -1303,7 +1328,11 @@ int main(int argc, char **argv) { config->max_anonymous_clients, config->keepalive_interval, config->keepalive_count, +#ifdef WITH_NET_IP config->mdns_adv ? config->mdns_name : NULL, +#else + NULL, +#endif remoteClientNew, NULL, remoteClientFree, @@ -1318,7 +1347,7 @@ int main(int argc, char **argv) { } if (daemonInitialize() < 0) { - ret = VIR_DAEMON_ERR_INIT; + ret = VIR_DAEMON_ERR_DRIVER; goto cleanup; } @@ -1442,7 +1471,10 @@ int main(int argc, char **argv) { sock_file, sock_file_ro, sock_file_adm, - ipsock, privileged) < 0) { +#ifdef WITH_NET_IP + ipsock, +#endif + privileged) < 0) { ret = VIR_DAEMON_ERR_NETWORK; goto cleanup; } diff --git a/src/remote/remote_daemon_config.c b/src/remote/remote_daemon_config.c index b1516befb4..9759cd9438 100644 --- a/src/remote/remote_daemon_config.c +++ b/src/remote/remote_daemon_config.c @@ -79,7 +79,7 @@ int daemonConfigFilePath(bool privileged, char **configfile) { if (privileged) { - if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/libvirtd.conf") < 0) + if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/" APP_NAME ".conf") < 0) goto error; } else { char *configdir = NULL; @@ -87,7 +87,7 @@ daemonConfigFilePath(bool privileged, char **configfile) if (!(configdir = virGetUserConfigDirectory())) goto error; - if (virAsprintf(configfile, "%s/libvirtd.conf", configdir) < 0) { + if (virAsprintf(configfile, "%s/" APP_NAME ".conf", configdir) < 0) { VIR_FREE(configdir); goto error; } @@ -104,18 +104,22 @@ struct daemonConfig* daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) { struct daemonConfig *data; +#ifdef WITH_NET_IP char *localhost; int ret; +#endif if (VIR_ALLOC(data) < 0) return NULL; +#ifdef WITH_NET_IP data->listen_tls = 1; data->listen_tcp = 0; if (VIR_STRDUP(data->tls_port, LIBVIRTD_TLS_PORT) < 0 || VIR_STRDUP(data->tcp_port, LIBVIRTD_TCP_PORT) < 0) goto error; +#endif /* Only default to PolicyKit if running as root */ #if WITH_POLKIT @@ -136,14 +140,16 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) VIR_STRDUP(data->unix_sock_admin_perms, "0700") < 0) goto error; -#if WITH_SASL +#ifdef WITH_NET_IP +# if WITH_SASL data->auth_tcp = REMOTE_AUTH_SASL; -#else +# else data->auth_tcp = REMOTE_AUTH_NONE; -#endif +# endif data->auth_tls = REMOTE_AUTH_NONE; data->mdns_adv = 0; +#endif data->min_workers = 5; data->max_workers = 20; @@ -172,6 +178,7 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) data->ovs_timeout = VIR_NETDEV_OVS_DEFAULT_TIMEOUT; +#ifdef WITH_NET_IP localhost = virGetHostname(); if (localhost == NULL) { /* we couldn't resolve the hostname; assume that we are @@ -190,6 +197,7 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED) VIR_FREE(localhost); if (ret < 0) goto error; +#endif return data; @@ -206,9 +214,11 @@ daemonConfigFree(struct daemonConfig *data) if (!data) return; +#ifdef WITH_NET_IP VIR_FREE(data->listen_addr); VIR_FREE(data->tls_port); VIR_FREE(data->tcp_port); +#endif tmp = data->access_drivers; while (tmp && *tmp) { VIR_FREE(*tmp); @@ -221,6 +231,7 @@ daemonConfigFree(struct daemonConfig *data) VIR_FREE(data->unix_sock_rw_perms); VIR_FREE(data->unix_sock_group); VIR_FREE(data->unix_sock_dir); +#ifdef WITH_NET_IP VIR_FREE(data->mdns_name); tmp = data->tls_allowed_dn_list; @@ -229,6 +240,7 @@ daemonConfigFree(struct daemonConfig *data) tmp++; } VIR_FREE(data->tls_allowed_dn_list); +#endif tmp = data->sasl_allowed_username_list; while (tmp && *tmp) { @@ -236,12 +248,15 @@ daemonConfigFree(struct daemonConfig *data) tmp++; } VIR_FREE(data->sasl_allowed_username_list); + +#ifdef WITH_NET_IP VIR_FREE(data->tls_priority); VIR_FREE(data->key_file); VIR_FREE(data->ca_file); VIR_FREE(data->cert_file); VIR_FREE(data->crl_file); +#endif VIR_FREE(data->host_uuid); VIR_FREE(data->host_uuid_source); @@ -256,6 +271,7 @@ daemonConfigLoadOptions(struct daemonConfig *data, const char *filename, virConfPtr conf) { +#ifdef WITH_NET_IP if (virConfGetValueBool(conf, "listen_tcp", &data->listen_tcp) < 0) goto error; if (virConfGetValueBool(conf, "listen_tls", &data->listen_tls) < 0) @@ -266,6 +282,7 @@ daemonConfigLoadOptions(struct daemonConfig *data, goto error; if (virConfGetValueString(conf, "listen_addr", &data->listen_addr) < 0) goto error; +#endif if (remoteConfigGetAuth(conf, filename, "auth_unix_rw", &data->auth_unix_rw) < 0) goto error; @@ -281,10 +298,12 @@ daemonConfigLoadOptions(struct daemonConfig *data, #endif if (remoteConfigGetAuth(conf, filename, "auth_unix_ro", &data->auth_unix_ro) < 0) goto error; +#ifdef WITH_NET_IP if (remoteConfigGetAuth(conf, filename, "auth_tcp", &data->auth_tcp) < 0) goto error; if (remoteConfigGetAuth(conf, filename, "auth_tls", &data->auth_tls) < 0) goto error; +#endif if (virConfGetValueStringList(conf, "access_drivers", false, &data->access_drivers) < 0) @@ -302,6 +321,7 @@ daemonConfigLoadOptions(struct daemonConfig *data, if (virConfGetValueString(conf, "unix_sock_dir", &data->unix_sock_dir) < 0) goto error; +#ifdef WITH_NET_IP if (virConfGetValueBool(conf, "mdns_adv", &data->mdns_adv) < 0) goto error; if (virConfGetValueString(conf, "mdns_name", &data->mdns_name) < 0) @@ -325,14 +345,14 @@ daemonConfigLoadOptions(struct daemonConfig *data, &data->tls_allowed_dn_list) < 0) goto error; + if (virConfGetValueString(conf, "tls_priority", &data->tls_priority) < 0) + goto error; +#endif if (virConfGetValueStringList(conf, "sasl_allowed_username_list", false, &data->sasl_allowed_username_list) < 0) goto error; - if (virConfGetValueString(conf, "tls_priority", &data->tls_priority) < 0) - goto error; - if (virConfGetValueUInt(conf, "min_workers", &data->min_workers) < 0) goto error; if (virConfGetValueUInt(conf, "max_workers", &data->max_workers) < 0) diff --git a/src/remote/remote_daemon_config.h b/src/remote/remote_daemon_config.h index 49ea80104b..86f78e756e 100644 --- a/src/remote/remote_daemon_config.h +++ b/src/remote/remote_daemon_config.h @@ -30,11 +30,13 @@ struct daemonConfig { char *host_uuid; char *host_uuid_source; +# ifdef WITH_NET_IP bool listen_tls; bool listen_tcp; char *listen_addr; char *tls_port; char *tcp_port; +# endif char *unix_sock_admin_perms; char *unix_sock_ro_perms; @@ -44,24 +46,29 @@ struct daemonConfig { int auth_unix_rw; int auth_unix_ro; +# ifdef WITH_NET_IP int auth_tcp; int auth_tls; +# endif char **access_drivers; +# ifdef WITH_NET_IP bool mdns_adv; char *mdns_name; bool tls_no_verify_certificate; bool tls_no_sanity_certificate; char **tls_allowed_dn_list; - char **sasl_allowed_username_list; char *tls_priority; char *key_file; char *cert_file; char *ca_file; char *crl_file; +# endif + + char **sasl_allowed_username_list; unsigned int min_workers; unsigned int max_workers; diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h index 4033a3cd2c..ef53179833 100644 --- a/src/remote/remote_driver.h +++ b/src/remote/remote_driver.h @@ -37,7 +37,6 @@ unsigned long remoteVersion(void); # define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock" # define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro" # define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock" -# define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf" /* Defaults for PKI directory. */ # define LIBVIRT_PKI_DIR SYSCONFDIR "/pki" -- 2.14.3

If we are running inside the daemon, the remote driver usually declines URIs without a hostname present to avoid connecting back to itself, and to avoid accepting a URI that would be handled by a real local driver later in the probe order. It is now, however, possible to connect to alternative local daemons so this special hack based on hostname is insufficiently flexible. Instead we are willing to connect to a local UNIX socket, provided the local daemon does not have a driver registered for the URI. In this case we will then connect to an alternative daemon with a socket path based on the URI. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.h | 2 + src/libvirt.c | 24 +++++++++ src/remote/remote_daemon_dispatch.c | 11 +++- src/remote/remote_driver.c | 100 ++++++++++++++++++++++++++---------- src/remote/remote_driver.h | 3 -- 5 files changed, 109 insertions(+), 31 deletions(-) diff --git a/src/driver.h b/src/driver.h index b4e50ab987..e8f0c0ebdf 100644 --- a/src/driver.h +++ b/src/driver.h @@ -107,6 +107,8 @@ int virSetSharedNWFilterDriver(virNWFilterDriverPtr driver) ATTRIBUTE_RETURN_CHE int virSetSharedSecretDriver(virSecretDriverPtr driver) ATTRIBUTE_RETURN_CHECK; int virSetSharedStorageDriver(virStorageDriverPtr driver) ATTRIBUTE_RETURN_CHECK; +bool virHasDriverForURIScheme(const char *scheme); + int virDriverLoadModule(const char *name, const char *regfunc, bool required); diff --git a/src/libvirt.c b/src/libvirt.c index 0a81cbfb99..d81b58aa10 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -689,6 +689,30 @@ virRegisterConnectDriver(virConnectDriverPtr driver, } +/** + * virHasDriverForURIScheme: + * @scheme: the URI scheme + * + * Determine if there is a driver registered that explicitly + * handles URIs with the scheme @scheme. + * + * Returns: true if a driver is registered + */ +bool virHasDriverForURIScheme(const char *scheme) +{ + size_t i, j; + for (i = 0; i < virConnectDriverTabCount; i++) { + if (!virConnectDriverTab[i]->uriSchemes) + continue; + for (j = 0; virConnectDriverTab[i]->uriSchemes[j]; j++) { + if (STREQ(virConnectDriverTab[i]->uriSchemes[j], scheme)) + return true; + } + } + + return false; +} + /** * virRegisterStateDriver: * @driver: pointer to a driver block diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index a8a5932d71..1eed9a0b38 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1833,7 +1833,16 @@ remoteDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED, priv->networkConn = virObjectRef(priv->conn); priv->nodedevConn = virObjectRef(priv->conn); priv->nwfilterConn = virObjectRef(priv->conn); - priv->secretConn = virObjectRef(priv->conn); + if (STRPREFIX(name, "secret:///")) { + priv->secretConn = virObjectRef(priv->conn); + } else { + const char *uri = geteuid() == 0 ? "secret:///system" : "secret:///session"; + priv->secretConn = flags & VIR_CONNECT_RO + ? virConnectOpenReadOnly(uri) + : virConnectOpen(uri); + if (!priv->secretConn) + goto cleanup; + } priv->storageConn = virObjectRef(priv->conn); /* force update the @readonly attribute which was inherited from the diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b4b034423a..a3ace44b06 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -51,6 +51,7 @@ #include "virauth.h" #include "virauthconfig.h" #include "virstring.h" +#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_REMOTE @@ -711,8 +712,7 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn, continue; \ } - -static char *remoteGetUNIXSocketNonRoot(void) +static char *remoteGetUNIXSocketNonRoot(const char *daemon_name) { char *sockname = NULL; char *userdir = virGetUserRuntimeDirectory(); @@ -720,23 +720,23 @@ static char *remoteGetUNIXSocketNonRoot(void) if (!userdir) return NULL; - if (virAsprintf(&sockname, "%s/" LIBVIRTD_USER_UNIX_SOCKET, userdir) < 0) { + if (virAsprintf(&sockname, "%s/%s-sock", userdir, + inside_daemon ? daemon_name : "libvirt") < 0) { VIR_FREE(userdir); return NULL; } - VIR_FREE(userdir); VIR_DEBUG("Chosen UNIX sockname %s", sockname); return sockname; } -static char *remoteGetUNIXSocketRoot(unsigned int flags) +static char *remoteGetUNIXSocketRoot(const char *daemon_name, unsigned int flags) { char *sockname = NULL; - if (VIR_STRDUP(sockname, - flags & VIR_DRV_OPEN_REMOTE_RO ? - LIBVIRTD_PRIV_UNIX_SOCKET_RO : LIBVIRTD_PRIV_UNIX_SOCKET) < 0) + if (virAsprintf(&sockname, "%s/%s-%s", LOCALSTATEDIR "/run/libvirt", + inside_daemon ? daemon_name : "libvirt", + flags & VIR_DRV_OPEN_REMOTE_RO ? "sock-ro" : "sock") < 0) return NULL; VIR_DEBUG("Chosen UNIX sockname %s", sockname); @@ -768,6 +768,8 @@ doRemoteOpen(virConnectPtr conn, struct private_data *priv, const char *driver_str, const char *transport_str, + const char *daemon_name, + const char *daemon_env, virConnectAuthPtr auth ATTRIBUTE_UNUSED, virConfPtr conf, unsigned int flags) @@ -1004,7 +1006,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (!(sockname = remoteGetUNIXSocketRoot(flags))) + if (!(sockname = remoteGetUNIXSocketRoot(daemon_name, flags))) goto failed; } @@ -1039,7 +1041,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (!(sockname = remoteGetUNIXSocketRoot(flags))) + if (!(sockname = remoteGetUNIXSocketRoot(daemon_name, flags))) goto failed; } @@ -1067,20 +1069,21 @@ doRemoteOpen(virConnectPtr conn, case trans_unix: if (!sockname) { if (flags & VIR_DRV_OPEN_REMOTE_USER) - sockname = remoteGetUNIXSocketNonRoot(); + sockname = remoteGetUNIXSocketNonRoot(daemon_name); else - sockname = remoteGetUNIXSocketRoot(flags); + sockname = remoteGetUNIXSocketRoot(daemon_name, flags); if (!sockname) goto failed; } - if ((flags & VIR_DRV_OPEN_REMOTE_AUTOSTART) && - !(daemonPath = virFileFindResourceFull("libvirtd", - NULL, NULL, - abs_topbuilddir "/src", - SBINDIR, - "LIBVIRTD_PATH"))) - goto failed; + if (flags & VIR_DRV_OPEN_REMOTE_AUTOSTART) { + if (!(daemonPath = virFileFindResourceFull(daemon_name, + NULL, NULL, + abs_topbuilddir "/src", + SBINDIR, + daemon_env))) + goto failed; + } if (!(priv->client = virNetClientNewUNIX(sockname, flags & VIR_DRV_OPEN_REMOTE_AUTOSTART, @@ -1104,7 +1107,7 @@ doRemoteOpen(virConnectPtr conn, goto failed; } - if (!(sockname = remoteGetUNIXSocketRoot(flags))) + if (!(sockname = remoteGetUNIXSocketRoot(daemon_name, flags))) goto failed; } @@ -1312,6 +1315,9 @@ remoteAllocPrivateData(void) return priv; } + +#define DRIVER_SCHEME_CHRS "abcdefghijklmnopqrstuvwxyz" + static virDrvOpenStatus remoteConnectOpen(virConnectPtr conn, virConnectAuthPtr auth, @@ -1324,14 +1330,53 @@ remoteConnectOpen(virConnectPtr conn, const char *autostart = virGetEnvBlockSUID("LIBVIRT_AUTOSTART"); char *driver = NULL; char *transport = NULL; + char *daemon_name = NULL; + char *daemon_env = NULL; - if (conn->uri && - remoteSplitURIScheme(conn->uri, &driver, &transport) < 0) - goto cleanup; + if (conn->uri) { + if (remoteSplitURIScheme(conn->uri, &driver, &transport) < 0) + goto cleanup; - if (inside_daemon && (!conn->uri || !conn->uri->server)) { - ret = VIR_DRV_OPEN_DECLINED; - goto cleanup; + if (strspn(driver, DRIVER_SCHEME_CHRS) < strlen(driver)) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid character in driver '%s'"), driver); + goto cleanup; + } + + if (inside_daemon) { + char *tmp; + if (virAsprintf(&daemon_name, "virt%sd", driver) < 0 || + virAsprintf(&daemon_env, "virt%sd_path", driver) < 0) + goto cleanup; + + tmp = daemon_env; + while (*tmp) { + *tmp = c_toupper(*tmp); + tmp++; + } + } else { + if (VIR_STRDUP(daemon_name, "libvirtd") < 0 || + VIR_STRDUP(daemon_env, "LIBVIRTD_PATH") < 0) + goto cleanup; + } + } + + if (inside_daemon) { + if (!conn->uri) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } + + /* + * If we're inside the daemon and there's a driver + * registered for this URI scheme, we should not + * handle this URI ourselves + */ + if (!conn->uri->server && + virHasDriverForURIScheme(driver)) { + ret = VIR_DRV_OPEN_DECLINED; + goto cleanup; + } } if (!(priv = remoteAllocPrivateData())) @@ -1378,7 +1423,8 @@ remoteConnectOpen(virConnectPtr conn, } } - ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags); + ret = doRemoteOpen(conn, priv, driver, transport, + daemon_name, daemon_env, auth, conf, rflags); if (ret != VIR_DRV_OPEN_SUCCESS) { conn->privateData = NULL; remoteDriverUnlock(priv); diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h index ef53179833..f6e3deb3d0 100644 --- a/src/remote/remote_driver.h +++ b/src/remote/remote_driver.h @@ -34,9 +34,6 @@ unsigned long remoteVersion(void); # define LIBVIRTD_LISTEN_ADDR NULL # define LIBVIRTD_TLS_PORT "16514" # define LIBVIRTD_TCP_PORT "16509" -# define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock" -# define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro" -# define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock" /* Defaults for PKI directory. */ # define LIBVIRT_PKI_DIR SYSCONFDIR "/pki" -- 2.14.3

The virtsecretd daemon will be responsible for providing the secret API driver functionality. The secret driver is now disabled for the main libvirtd daemon. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/Makefile.inc.am | 1 - src/secret/Makefile.inc.am | 54 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index a6c60a35b3..0a041b5638 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -148,7 +148,6 @@ libvirtd_CFLAGS = \ -DLOAD_INTERFACE \ -DLOAD_STORAGE \ -DLOAD_NODE_DEVICES \ - -DLOAD_SECRETS \ -DLOAD_NWFILTER \ -DLOAD_XEN \ -DLOAD_LIBXL \ diff --git a/src/secret/Makefile.inc.am b/src/secret/Makefile.inc.am index 305c4a1ead..aa947e85e9 100644 --- a/src/secret/Makefile.inc.am +++ b/src/secret/Makefile.inc.am @@ -35,4 +35,58 @@ libvirt_driver_secret_la_LIBADD = \ $(NULL) libvirt_driver_secret_la_LDFLAGS = $(AM_LDFLAGS_MOD_NOUNDEF) libvirt_driver_secret_la_SOURCES = $(SECRET_DRIVER_SOURCES) + +sbin_PROGRAMS += virtsecretd + +virtsecretd_SOURCES = $(LIBVIRTD_SOURCES) + +virtsecretd_CFLAGS = \ + $(LIBXML_CFLAGS) \ + $(GNUTLS_CFLAGS) \ + $(SASL_CFLAGS) \ + $(XDR_CFLAGS) \ + $(DBUS_CFLAGS) \ + $(LIBNL_CFLAGS) \ + $(WARN_CFLAGS) \ + $(PIE_CFLAGS) \ + $(COVERAGE_CFLAGS) \ + -I$(srcdir)/access \ + -I$(srcdir)/conf \ + -I$(srcdir)/rpc \ + -DSOCK_NAME="\"virtsecretd-sock\"" \ + -DSOCK_NAME_RO="\"virtsecretd-sock-ro\"" \ + -DSOCK_NAME_ADMIN="\"virtsecretd-admin-sock\"" \ + -DAPP_NAME="\"virtsecretd\"" \ + -DLOAD_SECRETS \ + -DREQUIRE_MODULE=true \ + $(NULL) + +virtsecretd_LDFLAGS = \ + $(RELRO_LDFLAGS) \ + $(PIE_LDFLAGS) \ + $(COVERAGE_LDFLAGS) \ + $(NO_INDIRECT_LDFLAGS) \ + $(NO_UNDEFINED_LDFLAGS) \ + $(NULL) + +virtsecretd_LDADD = \ + libvirt_driver_admin.la \ + libvirt-lxc.la \ + libvirt-qemu.la \ + libvirt.la \ + $(LIBXML_LIBS) \ + $(GNUTLS_LIBS) \ + $(SASL_LIBS) \ + $(DBUS_LIBS) \ + $(LIBNL_LIBS) \ + $(NULL) + +if WITH_DTRACE_PROBES +virtsecretd_LDADD += ../src/libvirt_probes.lo +endif WITH_DTRACE_PROBES + +virtsecretd_LDADD += \ + ../gnulib/lib/libgnu.la \ + $(LIBSOCKET) \ + $(NULL) endif WITH_SECRETS -- 2.14.3
participants (2)
-
Daniel P. Berrangé
-
Michal Privoznik