[libvirt] [PATCH 0/6] Split the storage file driver impls from the storage driver backends

The storage driver backends currently register storage file backends as a side effect of initialization. This is fine right now while both run in the same libvirtd process, but in future the storage file backends will need to run in the hypervisor daemon which will be separate from the storage daemon. This small series splits them, makin the storage file backends be registered "on demand" Daniel P. Berrangé (6): util: create new virmodule.{c,h} files for dlopen support code storage: split gluster storage file code from storage driver backend storage: split fs storage file code from storage driver backend storage: fix virStorageFileGetBackingStoreStr error handling util: refactor storage file checks to allow error reporting storage: create separate loadable modules for storage file drivers libvirt.spec.in | 4 + src/driver.c | 133 +----------- src/driver.h | 3 - src/libvirt_driver_modules.syms | 1 - src/libvirt_private.syms | 3 + src/qemu/qemu_domain.c | 21 +- src/qemu/qemu_driver.c | 21 +- src/storage/Makefile.inc.am | 44 ++++ src/storage/storage_backend.c | 3 +- src/storage/storage_backend_fs.c | 210 +------------------ src/storage/storage_backend_gluster.c | 305 +--------------------------- src/storage/storage_backend_gluster.h | 2 +- src/storage/storage_file_fs.c | 251 +++++++++++++++++++++++ src/storage/storage_file_fs.h | 29 +++ src/storage/storage_file_gluster.c | 366 ++++++++++++++++++++++++++++++++++ src/storage/storage_file_gluster.h | 27 +++ src/util/Makefile.inc.am | 2 + src/util/virmodule.c | 163 +++++++++++++++ src/util/virmodule.h | 29 +++ src/util/virstoragefile.c | 123 ++++++++---- src/util/virstoragefile.h | 9 +- src/util/virstoragefilebackend.c | 72 +++++-- src/util/virstoragefilebackend.h | 9 +- 23 files changed, 1098 insertions(+), 732 deletions(-) create mode 100644 src/storage/storage_file_fs.c create mode 100644 src/storage/storage_file_fs.h create mode 100644 src/storage/storage_file_gluster.c create mode 100644 src/storage/storage_file_gluster.h create mode 100644 src/util/virmodule.c create mode 100644 src/util/virmodule.h -- 2.14.3

The driver.{c,h} files are primarily targetted at loading hypervisor drivers and some helper functions in that area. It also, however, contains a generically useful function for loading extension modules that is called by the storage driver. Split that functionality off into a new virmodule.{c,h} file to isolate it. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 133 +------------------------------- src/driver.h | 3 - src/libvirt_driver_modules.syms | 1 - src/libvirt_private.syms | 3 + src/storage/storage_backend.c | 3 +- src/util/Makefile.inc.am | 2 + src/util/virmodule.c | 163 ++++++++++++++++++++++++++++++++++++++++ src/util/virmodule.h | 29 +++++++ 8 files changed, 201 insertions(+), 136 deletions(-) create mode 100644 src/util/virmodule.c create mode 100644 src/util/virmodule.h diff --git a/src/driver.c b/src/driver.c index 447f61d554..8b5ade763f 100644 --- a/src/driver.c +++ b/src/driver.c @@ -28,6 +28,7 @@ #include "viralloc.h" #include "virfile.h" #include "virlog.h" +#include "virmodule.h" #include "virthread.h" #include "configmake.h" @@ -38,136 +39,6 @@ VIR_LOG_INIT("driver"); /* XXX re-implement this for other OS, or use libtools helper lib ? */ #define DEFAULT_DRIVER_DIR LIBDIR "/libvirt/connection-driver" -#ifdef HAVE_DLFCN_H -# include <dlfcn.h> - - -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); - - virUpdateSelfLastChanged(file); - - if (!(handle = dlopen(file, flags))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to load module '%s': %s"), file, dlerror()); - return NULL; - } - - return handle; -} - - -static void * -virDriverLoadModuleFunc(void *handle, - const char *file, - const char *funcname) -{ - void *regsym; - - VIR_DEBUG("Lookup function '%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; -} - - -/** - * virDriverLoadModuleFull: - * @path: filename of module to load - * @regfunc: name of the function that registers the module - * - * Loads a loadable module named @path and calls the - * 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). - * - * Returns 0 on success, 1 if the module was not found and -1 on any error. - */ -int -virDriverLoadModuleFull(const char *path, - const char *regfunc, - bool required) -{ - void *rethandle = NULL; - int (*regsym)(void); - int ret = -1; - - if (!virFileExists(path)) { - 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))) - goto cleanup; - - if (!(regsym = virDriverLoadModuleFunc(rethandle, path, regfunc))) - goto cleanup; - - if ((*regsym)() < 0) { - /* 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; - } - - rethandle = NULL; - ret = 0; - - cleanup: - if (rethandle) - dlclose(rethandle); - return ret; -} - -#else /* ! HAVE_DLFCN_H */ -int -virDriverLoadModuleFull(const char *path ATTRIBUTE_UNUSED, - const char *regfunc ATTRIBUTE_UNUSED, - bool required) -{ - VIR_DEBUG("dlopen not available on this platform"); - 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 @@ -188,7 +59,7 @@ virDriverLoadModule(const char *name, "LIBVIRT_DRIVER_DIR"))) return -1; - ret = virDriverLoadModuleFull(modfile, regfunc, required); + ret = virModuleLoad(modfile, regfunc, required); VIR_FREE(modfile); diff --git a/src/driver.h b/src/driver.h index b4e50ab987..0b1f7a2269 100644 --- a/src/driver.h +++ b/src/driver.h @@ -110,9 +110,6 @@ int virSetSharedStorageDriver(virStorageDriverPtr driver) ATTRIBUTE_RETURN_CHECK int virDriverLoadModule(const char *name, const char *regfunc, bool required); -int virDriverLoadModuleFull(const char *path, - const char *regfunc, - bool required); virConnectPtr virGetConnectInterface(void); virConnectPtr virGetConnectNetwork(void); diff --git a/src/libvirt_driver_modules.syms b/src/libvirt_driver_modules.syms index bd9bf1c315..f9d0ee9b97 100644 --- a/src/libvirt_driver_modules.syms +++ b/src/libvirt_driver_modules.syms @@ -4,7 +4,6 @@ # driver.h virDriverLoadModule; -virDriverLoadModuleFull; # Let emacs know we want case-insensitive sorting # Local Variables: diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d2728749fb..1051a105b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2228,6 +2228,9 @@ virMediatedDeviceTypeFree; virMediatedDeviceTypeReadAttrs; +# util/virmodule.h +virModuleLoad; + # util/virnetdev.h virNetDevAddMulti; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index cb1bcc0944..7d226f3d3a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -33,6 +33,7 @@ #include "virstoragefile.h" #include "storage_backend.h" #include "virlog.h" +#include "virmodule.h" #include "virfile.h" #include "configmake.h" @@ -97,7 +98,7 @@ virStorageDriverLoadBackendModule(const char *name, "LIBVIRT_STORAGE_BACKEND_DIR"))) return -1; - ret = virDriverLoadModuleFull(modfile, regfunc, forceload); + ret = virModuleLoad(modfile, regfunc, forceload); VIR_FREE(modfile); diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am index 9624fb687c..ec8745da7e 100644 --- a/src/util/Makefile.inc.am +++ b/src/util/Makefile.inc.am @@ -100,6 +100,8 @@ UTIL_SOURCES = \ util/virmacaddr.h \ util/virmacmap.c \ util/virmacmap.h \ + util/virmodule.c \ + util/virmodule.h \ util/virnetdev.c \ util/virnetdev.h \ util/virnetdevbandwidth.c \ diff --git a/src/util/virmodule.c b/src/util/virmodule.c new file mode 100644 index 0000000000..ff8c22752e --- /dev/null +++ b/src/util/virmodule.c @@ -0,0 +1,163 @@ +/* + * virmodule.c: APIs for dlopen'ing extension modules + * + * Copyright (C) 2012-2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include "internal.h" +#include "virmodule.h" +#include "virerror.h" +#include "virfile.h" +#include "virlog.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("util.module"); + +#ifdef HAVE_DLFCN_H +# include <dlfcn.h> + +static void * +virModuleLoadFile(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); + + virUpdateSelfLastChanged(file); + + if (!(handle = dlopen(file, flags))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load module '%s': %s"), file, dlerror()); + return NULL; + } + + return handle; +} + + +static void * +virModuleLoadFunc(void *handle, + const char *file, + const char *funcname) +{ + void *regsym; + + VIR_DEBUG("Lookup function '%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; +} + + +/** + * virModuleLoad: + * @path: filename of module to load + * @regfunc: name of the function that registers the module + * @required: true if module must exist on disk, false to silently skip + * + * Loads a loadable module named @path and calls the + * 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). + * + * Returns 0 on success, 1 if the module was not found and -1 on any error. + */ +int +virModuleLoad(const char *path, + const char *regfunc, + bool required) +{ + void *rethandle = NULL; + int (*regsym)(void); + int ret = -1; + + if (!virFileExists(path)) { + 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 = virModuleLoadFile(path))) + goto cleanup; + + if (!(regsym = virModuleLoadFunc(rethandle, path, regfunc))) + goto cleanup; + + if ((*regsym)() < 0) { + /* 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; + } + + rethandle = NULL; + ret = 0; + + cleanup: + if (rethandle) + dlclose(rethandle); + return ret; +} + +#else /* ! HAVE_DLFCN_H */ +int +virModuleLoad(const char *path ATTRIBUTE_UNUSED, + const char *regfunc ATTRIBUTE_UNUSED, + bool required) +{ + VIR_DEBUG("dlopen not available on this platform"); + 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 */ diff --git a/src/util/virmodule.h b/src/util/virmodule.h new file mode 100644 index 0000000000..cccd836b41 --- /dev/null +++ b/src/util/virmodule.h @@ -0,0 +1,29 @@ +/* + * virmodule.h: APIs for dlopen'ing extension modules + * + * Copyright (C) 2012-2018 Red Hat, Inc. + * + * 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/>. + * + */ + +#ifndef __VIR_MODULE_H__ +# define __VIR_MODULE_H__ + +int virModuleLoad(const char *path, + const char *regfunc, + bool required); + +#endif /* __VIR_MODULE_H__ */ -- 2.14.3

On Wed, Apr 25, 2018 at 16:52:38 +0100, Daniel Berrange wrote:
The driver.{c,h} files are primarily targetted at loading hypervisor drivers and some helper functions in that area. It also, however, contains a generically useful function for loading extension modules that is called by the storage driver. Split that functionality off into a new virmodule.{c,h} file to isolate it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/driver.c | 133 +------------------------------- src/driver.h | 3 - src/libvirt_driver_modules.syms | 1 - src/libvirt_private.syms | 3 + src/storage/storage_backend.c | 3 +- src/util/Makefile.inc.am | 2 + src/util/virmodule.c | 163 ++++++++++++++++++++++++++++++++++++++++ src/util/virmodule.h | 29 +++++++ 8 files changed, 201 insertions(+), 136 deletions(-) create mode 100644 src/util/virmodule.c create mode 100644 src/util/virmodule.h
ACK

The storage file code needs to be run in the hypervisor drivers, while the storage backend code needs to be run in the storage driver. Split the source code as a preparatory step for creating separate loadable modules. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/Makefile.inc.am | 2 + src/storage/storage_backend_gluster.c | 305 +--------------------------- src/storage/storage_backend_gluster.h | 2 +- src/storage/storage_file_gluster.c | 366 ++++++++++++++++++++++++++++++++++ src/storage/storage_file_gluster.h | 27 +++ 5 files changed, 399 insertions(+), 303 deletions(-) create mode 100644 src/storage/storage_file_gluster.c create mode 100644 src/storage/storage_file_gluster.h diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am index ae9fdeb6a9..1e81249272 100644 --- a/src/storage/Makefile.inc.am +++ b/src/storage/Makefile.inc.am @@ -55,6 +55,8 @@ STORAGE_DRIVER_SHEEPDOG_SOURCES = \ STORAGE_DRIVER_GLUSTER_SOURCES = \ storage/storage_backend_gluster.h \ storage/storage_backend_gluster.c \ + storage/storage_file_gluster.h \ + storage/storage_file_gluster.c \ $(NULL) STORAGE_DRIVER_ZFS_SOURCES = \ diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index c6cc531e2f..aca772676c 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -1,7 +1,7 @@ /* * storage_backend_gluster.c: storage backend for Gluster handling * - * Copyright (C) 2013-2014 Red Hat, Inc. + * Copyright (C) 2013-2018 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -24,11 +24,11 @@ #include <glusterfs/api/glfs.h> #include "storage_backend_gluster.h" +#include "storage_file_gluster.h" #include "storage_conf.h" #include "viralloc.h" #include "virerror.h" #include "virlog.h" -#include "virstoragefilebackend.h" #include "virstring.h" #include "viruri.h" #include "storage_util.h" @@ -561,312 +561,13 @@ virStorageBackend virStorageBackendGluster = { }; -typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; -typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; - -struct _virStorageFileBackendGlusterPriv { - glfs_t *vol; - char *canonpath; -}; - - -static void -virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - - VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%u/%s%s)", - src, src->hosts->name, src->hosts->port, src->volume, src->path); - - if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv->canonpath); - - VIR_FREE(priv); - src->drv->priv = NULL; -} - -static int -virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, - virStorageNetHostDefPtr host) -{ - const char *transport = virStorageNetHostTransportTypeToString(host->transport); - const char *hoststr = NULL; - int port = 0; - - switch ((virStorageNetHostTransport) host->transport) { - case VIR_STORAGE_NET_HOST_TRANS_RDMA: - case VIR_STORAGE_NET_HOST_TRANS_TCP: - hoststr = host->name; - port = host->port; - break; - - case VIR_STORAGE_NET_HOST_TRANS_UNIX: - hoststr = host->socket; - break; - - case VIR_STORAGE_NET_HOST_TRANS_LAST: - break; - } - - VIR_DEBUG("adding gluster host for %p: transport=%s host=%s port=%d", - priv, transport, hoststr, port); - - if (glfs_set_volfile_server(priv->vol, transport, hoststr, port) < 0) { - virReportSystemError(errno, - _("failed to set gluster volfile server '%s'"), - hoststr); - return -1; - } - - return 0; -} - - -static int -virStorageFileBackendGlusterInit(virStorageSourcePtr src) -{ - virStorageFileBackendGlusterPrivPtr priv = NULL; - size_t i; - - if (!src->volume) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing gluster volume name for path '%s'"), - src->path); - return -1; - } - - if (VIR_ALLOC(priv) < 0) - return -1; - - VIR_DEBUG("initializing gluster storage file %p " - "(priv='%p' volume='%s' path='%s') as [%u:%u]", - src, priv, src->volume, src->path, - (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); - - if (!(priv->vol = glfs_new(src->volume))) { - virReportOOMError(); - goto error; - } - - for (i = 0; i < src->nhosts; i++) { - if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) - goto error; - } - - if (glfs_init(priv->vol) < 0) { - virReportSystemError(errno, - _("failed to initialize gluster connection " - "(src=%p priv=%p)"), src, priv); - goto error; - } - - src->drv->priv = priv; - - return 0; - - error: - if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv); - - return -1; -} - - -static int -virStorageFileBackendGlusterCreate(virStorageSourcePtr src) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - glfs_fd_t *fd = NULL; - mode_t mode = S_IRUSR; - - if (!src->readonly) - mode |= S_IWUSR; - - if (!(fd = glfs_creat(priv->vol, src->path, - O_CREAT | O_TRUNC | O_WRONLY, mode))) - return -1; - - ignore_value(glfs_close(fd)); - return 0; -} - - -static int -virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - - return glfs_unlink(priv->vol, src->path); -} - - -static int -virStorageFileBackendGlusterStat(virStorageSourcePtr src, - struct stat *st) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - - return glfs_stat(priv->vol, src->path, st); -} - - -static ssize_t -virStorageFileBackendGlusterRead(virStorageSourcePtr src, - size_t offset, - size_t len, - char **buf) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - glfs_fd_t *fd = NULL; - ssize_t ret = -1; - - *buf = NULL; - - if (!(fd = glfs_open(priv->vol, src->path, O_RDONLY))) { - virReportSystemError(errno, _("Failed to open file '%s'"), - src->path); - return -1; - } - - if (offset > 0) { - if (glfs_lseek(fd, offset, SEEK_SET) == (off_t) -1) { - virReportSystemError(errno, _("cannot seek into '%s'"), src->path); - goto cleanup; - } - } - - ret = virStorageBackendGlusterRead(fd, src->path, len, buf); - - cleanup: - if (fd) - glfs_close(fd); - - return ret; -} - - -static int -virStorageFileBackendGlusterAccess(virStorageSourcePtr src, - int mode) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - - return glfs_access(priv->vol, src->path, mode); -} - -static int -virStorageFileBackendGlusterReadlinkCallback(const char *path, - char **linkpath, - void *data) -{ - virStorageFileBackendGlusterPrivPtr priv = data; - char *buf = NULL; - size_t bufsiz = 0; - ssize_t ret; - struct stat st; - - *linkpath = NULL; - - if (glfs_stat(priv->vol, path, &st) < 0) { - virReportSystemError(errno, - _("failed to stat gluster path '%s'"), - path); - return -1; - } - - if (!S_ISLNK(st.st_mode)) - return 1; - - realloc: - if (VIR_EXPAND_N(buf, bufsiz, 256) < 0) - goto error; - - if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) { - virReportSystemError(errno, - _("failed to read link of gluster file '%s'"), - path); - goto error; - } - - if (ret == bufsiz) - goto realloc; - - buf[ret] = '\0'; - - *linkpath = buf; - - return 0; - - error: - VIR_FREE(buf); - return -1; -} - - -static const char * -virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - char *filePath = NULL; - - if (priv->canonpath) - return priv->canonpath; - - if (!(filePath = virStorageFileCanonicalizePath(src->path, - virStorageFileBackendGlusterReadlinkCallback, - priv))) - return NULL; - - ignore_value(virAsprintf(&priv->canonpath, "gluster://%s:%u/%s/%s", - src->hosts->name, - src->hosts->port, - src->volume, - filePath)); - - VIR_FREE(filePath); - - return priv->canonpath; -} - - -static int -virStorageFileBackendGlusterChown(const virStorageSource *src, - uid_t uid, - gid_t gid) -{ - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; - - return glfs_chown(priv->vol, src->path, uid, gid); -} - - -virStorageFileBackend virStorageFileBackendGluster = { - .type = VIR_STORAGE_TYPE_NETWORK, - .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER, - - .backendInit = virStorageFileBackendGlusterInit, - .backendDeinit = virStorageFileBackendGlusterDeinit, - - .storageFileCreate = virStorageFileBackendGlusterCreate, - .storageFileUnlink = virStorageFileBackendGlusterUnlink, - .storageFileStat = virStorageFileBackendGlusterStat, - .storageFileRead = virStorageFileBackendGlusterRead, - .storageFileAccess = virStorageFileBackendGlusterAccess, - .storageFileChown = virStorageFileBackendGlusterChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, -}; - - int virStorageBackendGlusterRegister(void) { if (virStorageBackendRegister(&virStorageBackendGluster) < 0) return -1; - if (virStorageFileBackendRegister(&virStorageFileBackendGluster) < 0) + if (virStorageFileGlusterRegister() < 0) return -1; return 0; diff --git a/src/storage/storage_backend_gluster.h b/src/storage/storage_backend_gluster.h index 91b8d8275d..12a1c04f8d 100644 --- a/src/storage/storage_backend_gluster.h +++ b/src/storage/storage_backend_gluster.h @@ -1,7 +1,7 @@ /* * storage_backend_gluster.h: storage backend for Gluster handling * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2018 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public diff --git a/src/storage/storage_file_gluster.c b/src/storage/storage_file_gluster.c new file mode 100644 index 0000000000..f8bbde8cfe --- /dev/null +++ b/src/storage/storage_file_gluster.c @@ -0,0 +1,366 @@ +/* + * storage_file_gluster.c: storage file backend for Gluster handling + * + * Copyright (C) 2013-2018 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <glusterfs/api/glfs.h> + +#include "storage_file_gluster.h" +#include "viralloc.h" +#include "virerror.h" +#include "virlog.h" +#include "virstoragefilebackend.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +VIR_LOG_INIT("storage.storage_file_gluster"); + + +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; + +struct _virStorageFileBackendGlusterPriv { + glfs_t *vol; + char *canonpath; +}; + +static void +virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%u/%s%s)", + src, src->hosts->name, src->hosts->port, src->volume, src->path); + + if (priv->vol) + glfs_fini(priv->vol); + VIR_FREE(priv->canonpath); + + VIR_FREE(priv); + src->drv->priv = NULL; +} + +static int +virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, + virStorageNetHostDefPtr host) +{ + const char *transport = virStorageNetHostTransportTypeToString(host->transport); + const char *hoststr = NULL; + int port = 0; + + switch ((virStorageNetHostTransport) host->transport) { + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + case VIR_STORAGE_NET_HOST_TRANS_TCP: + hoststr = host->name; + port = host->port; + break; + + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + hoststr = host->socket; + break; + + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; + } + + VIR_DEBUG("adding gluster host for %p: transport=%s host=%s port=%d", + priv, transport, hoststr, port); + + if (glfs_set_volfile_server(priv->vol, transport, hoststr, port) < 0) { + virReportSystemError(errno, + _("failed to set gluster volfile server '%s'"), + hoststr); + return -1; + } + + return 0; +} + + +static int +virStorageFileBackendGlusterInit(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = NULL; + size_t i; + + if (!src->volume) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing gluster volume name for path '%s'"), + src->path); + return -1; + } + + if (VIR_ALLOC(priv) < 0) + return -1; + + VIR_DEBUG("initializing gluster storage file %p " + "(priv='%p' volume='%s' path='%s') as [%u:%u]", + src, priv, src->volume, src->path, + (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); + + if (!(priv->vol = glfs_new(src->volume))) { + virReportOOMError(); + goto error; + } + + for (i = 0; i < src->nhosts; i++) { + if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) + goto error; + } + + if (glfs_init(priv->vol) < 0) { + virReportSystemError(errno, + _("failed to initialize gluster connection " + "(src=%p priv=%p)"), src, priv); + goto error; + } + + src->drv->priv = priv; + + return 0; + + error: + if (priv->vol) + glfs_fini(priv->vol); + VIR_FREE(priv); + + return -1; +} + + +static int +virStorageFileBackendGlusterCreate(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + mode_t mode = S_IRUSR; + + if (!src->readonly) + mode |= S_IWUSR; + + if (!(fd = glfs_creat(priv->vol, src->path, + O_CREAT | O_TRUNC | O_WRONLY, mode))) + return -1; + + ignore_value(glfs_close(fd)); + return 0; +} + + +static int +virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_unlink(priv->vol, src->path); +} + + +static int +virStorageFileBackendGlusterStat(virStorageSourcePtr src, + struct stat *st) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_stat(priv->vol, src->path, st); +} + + +static ssize_t +virStorageFileBackendGlusterRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + ssize_t ret = -1; + char *s; + size_t nread = 0; + + *buf = NULL; + + if (!(fd = glfs_open(priv->vol, src->path, O_RDONLY))) { + virReportSystemError(errno, _("Failed to open file '%s'"), + src->path); + return -1; + } + + if (offset > 0) { + if (glfs_lseek(fd, offset, SEEK_SET) == (off_t) -1) { + virReportSystemError(errno, _("cannot seek into '%s'"), src->path); + goto cleanup; + } + } + + + if (VIR_ALLOC_N(*buf, len) < 0) + return -1; + + s = *buf; + while (len) { + ssize_t r = glfs_read(fd, s, len, 0); + if (r < 0 && errno == EINTR) + continue; + if (r < 0) { + VIR_FREE(*buf); + virReportSystemError(errno, _("unable to read '%s'"), src->path); + return r; + } + if (r == 0) + return nread; + s += r; + len -= r; + nread += r; + } + + ret = nread; + + cleanup: + if (fd) + glfs_close(fd); + + return ret; +} + + +static int +virStorageFileBackendGlusterAccess(virStorageSourcePtr src, + int mode) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_access(priv->vol, src->path, mode); +} + +static int +virStorageFileBackendGlusterReadlinkCallback(const char *path, + char **linkpath, + void *data) +{ + virStorageFileBackendGlusterPrivPtr priv = data; + char *buf = NULL; + size_t bufsiz = 0; + ssize_t ret; + struct stat st; + + *linkpath = NULL; + + if (glfs_stat(priv->vol, path, &st) < 0) { + virReportSystemError(errno, + _("failed to stat gluster path '%s'"), + path); + return -1; + } + + if (!S_ISLNK(st.st_mode)) + return 1; + + realloc: + if (VIR_EXPAND_N(buf, bufsiz, 256) < 0) + goto error; + + if ((ret = glfs_readlink(priv->vol, path, buf, bufsiz)) < 0) { + virReportSystemError(errno, + _("failed to read link of gluster file '%s'"), + path); + goto error; + } + + if (ret == bufsiz) + goto realloc; + + buf[ret] = '\0'; + + *linkpath = buf; + + return 0; + + error: + VIR_FREE(buf); + return -1; +} + + +static const char * +virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + char *filePath = NULL; + + if (priv->canonpath) + return priv->canonpath; + + if (!(filePath = virStorageFileCanonicalizePath(src->path, + virStorageFileBackendGlusterReadlinkCallback, + priv))) + return NULL; + + ignore_value(virAsprintf(&priv->canonpath, "gluster://%s:%u/%s/%s", + src->hosts->name, + src->hosts->port, + src->volume, + filePath)); + + VIR_FREE(filePath); + + return priv->canonpath; +} + + +static int +virStorageFileBackendGlusterChown(const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_chown(priv->vol, src->path, uid, gid); +} + + +virStorageFileBackend virStorageFileBackendGluster = { + .type = VIR_STORAGE_TYPE_NETWORK, + .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER, + + .backendInit = virStorageFileBackendGlusterInit, + .backendDeinit = virStorageFileBackendGlusterDeinit, + + .storageFileCreate = virStorageFileBackendGlusterCreate, + .storageFileUnlink = virStorageFileBackendGlusterUnlink, + .storageFileStat = virStorageFileBackendGlusterStat, + .storageFileRead = virStorageFileBackendGlusterRead, + .storageFileAccess = virStorageFileBackendGlusterAccess, + .storageFileChown = virStorageFileBackendGlusterChown, + + .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, +}; + + +int +virStorageFileGlusterRegister(void) +{ + if (virStorageFileBackendRegister(&virStorageFileBackendGluster) < 0) + return -1; + + return 0; +} diff --git a/src/storage/storage_file_gluster.h b/src/storage/storage_file_gluster.h new file mode 100644 index 0000000000..572254aedb --- /dev/null +++ b/src/storage/storage_file_gluster.h @@ -0,0 +1,27 @@ +/* + * storage_file_gluster.h: storage file backend for Gluster handling + * + * Copyright (C) 2013-2018 Red Hat, Inc. + * + * 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/>. + * + */ + +#ifndef __VIR_STORAGE_FILE_GLUSTER_H__ +# define __VIR_STORAGE_FILE_GLUSTER_H__ + +int virStorageFileGlusterRegister(void); + +#endif /* __VIR_STORAGE_FILE_GLUSTER_H__ */ -- 2.14.3

On Wed, Apr 25, 2018 at 16:52:39 +0100, Daniel Berrange wrote:
The storage file code needs to be run in the hypervisor drivers, while the storage backend code needs to be run in the storage driver. Split the source code as a preparatory step for creating separate loadable modules.
At first I thought it might be better to stash this into the storage driver, but when I went through this series I agree that this is not suited for the storage driver. One of the benefits of having this separately would be that e.g. gluster will not leak it's stuff around the main qemu driver. Theoretically that could be achieved by yet another daemon for this functionality.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/Makefile.inc.am | 2 + src/storage/storage_backend_gluster.c | 305 +--------------------------- src/storage/storage_backend_gluster.h | 2 +- src/storage/storage_file_gluster.c | 366 ++++++++++++++++++++++++++++++++++ src/storage/storage_file_gluster.h | 27 +++ 5 files changed, 399 insertions(+), 303 deletions(-) create mode 100644 src/storage/storage_file_gluster.c create mode 100644 src/storage/storage_file_gluster.h
[...]
diff --git a/src/storage/storage_backend_gluster.h b/src/storage/storage_backend_gluster.h index 91b8d8275d..12a1c04f8d 100644 --- a/src/storage/storage_backend_gluster.h +++ b/src/storage/storage_backend_gluster.h @@ -1,7 +1,7 @@ /* * storage_backend_gluster.h: storage backend for Gluster handling * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2018 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public
You did not touch anything in this file other than the copyright. [...]
diff --git a/src/storage/storage_file_gluster.h b/src/storage/storage_file_gluster.h new file mode 100644 index 0000000000..572254aedb --- /dev/null +++ b/src/storage/storage_file_gluster.h @@ -0,0 +1,27 @@ +/* + * storage_file_gluster.h: storage file backend for Gluster handling + * + * Copyright (C) 2013-2018 Red Hat, Inc.
There is no code from 2013 in this file yet, since the function is new.
+ * + * 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
ACK

On Thu, Apr 26, 2018 at 09:39:44AM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 16:52:39 +0100, Daniel Berrange wrote:
The storage file code needs to be run in the hypervisor drivers, while the storage backend code needs to be run in the storage driver. Split the source code as a preparatory step for creating separate loadable modules.
At first I thought it might be better to stash this into the storage driver, but when I went through this series I agree that this is not suited for the storage driver.
One of the benefits of having this separately would be that e.g. gluster will not leak it's stuff around the main qemu driver. Theoretically that could be achieved by yet another daemon for this functionality.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/Makefile.inc.am | 2 + src/storage/storage_backend_gluster.c | 305 +--------------------------- src/storage/storage_backend_gluster.h | 2 +- src/storage/storage_file_gluster.c | 366 ++++++++++++++++++++++++++++++++++ src/storage/storage_file_gluster.h | 27 +++ 5 files changed, 399 insertions(+), 303 deletions(-) create mode 100644 src/storage/storage_file_gluster.c create mode 100644 src/storage/storage_file_gluster.h
[...]
diff --git a/src/storage/storage_backend_gluster.h b/src/storage/storage_backend_gluster.h index 91b8d8275d..12a1c04f8d 100644 --- a/src/storage/storage_backend_gluster.h +++ b/src/storage/storage_backend_gluster.h @@ -1,7 +1,7 @@ /* * storage_backend_gluster.h: storage backend for Gluster handling * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2018 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public
You did not touch anything in this file other than the copyright.
[...]
Habit of touching date in all files in a commit...
diff --git a/src/storage/storage_file_gluster.h b/src/storage/storage_file_gluster.h new file mode 100644 index 0000000000..572254aedb --- /dev/null +++ b/src/storage/storage_file_gluster.h @@ -0,0 +1,27 @@ +/* + * storage_file_gluster.h: storage file backend for Gluster handling + * + * Copyright (C) 2013-2018 Red Hat, Inc.
There is no code from 2013 in this file yet, since the function is new.
Opps, cut & paste from the other file.
+ * + * 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
ACK
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 storage file code needs to be run in the hypervisor drivers, while the storage backend code needs to be run in the storage driver. Split the source code as a preparatory step for creating separate loadable modules. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/Makefile.inc.am | 2 + src/storage/storage_backend_fs.c | 210 +------------------------------- src/storage/storage_file_fs.c | 251 +++++++++++++++++++++++++++++++++++++++ src/storage/storage_file_fs.h | 29 +++++ 4 files changed, 285 insertions(+), 207 deletions(-) create mode 100644 src/storage/storage_file_fs.c create mode 100644 src/storage/storage_file_fs.h diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am index 1e81249272..af2c97ab93 100644 --- a/src/storage/Makefile.inc.am +++ b/src/storage/Makefile.inc.am @@ -14,6 +14,8 @@ STORAGE_DRIVER_SOURCES = \ STORAGE_DRIVER_FS_SOURCES = \ storage/storage_backend_fs.h \ storage/storage_backend_fs.c \ + storage/storage_file_fs.h \ + storage/storage_file_fs.c \ $(NULL) STORAGE_DRIVER_LVM_SOURCES = \ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 9b0fcf92c5..bface86b43 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2015 Red Hat, Inc. + * Copyright (C) 2007-2018 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -28,21 +28,15 @@ #include <stdio.h> #include <errno.h> #include <fcntl.h> -#include <unistd.h> #include <string.h> -#include <libxml/parser.h> -#include <libxml/tree.h> -#include <libxml/xpath.h> - #include "virerror.h" #include "storage_backend_fs.h" +#include "storage_file_fs.h" #include "storage_util.h" #include "storage_conf.h" -#include "virstoragefilebackend.h" #include "vircommand.h" #include "viralloc.h" -#include "virxml.h" #include "virfile.h" #include "virlog.h" #include "virstring.h" @@ -705,198 +699,6 @@ virStorageBackend virStorageBackendNetFileSystem = { #endif /* WITH_STORAGE_FS */ -typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv; -typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr; - -struct _virStorageFileBackendFsPriv { - char *canonpath; /* unique file identifier (canonical path) */ -}; - - -static void -virStorageFileBackendFileDeinit(virStorageSourcePtr src) -{ - VIR_DEBUG("deinitializing FS storage file %p (%s:%s)", src, - virStorageTypeToString(virStorageSourceGetActualType(src)), - src->path); - - virStorageFileBackendFsPrivPtr priv = src->drv->priv; - - VIR_FREE(priv->canonpath); - VIR_FREE(priv); -} - - -static int -virStorageFileBackendFileInit(virStorageSourcePtr src) -{ - virStorageFileBackendFsPrivPtr priv = NULL; - - VIR_DEBUG("initializing FS storage file %p (%s:%s)[%u:%u]", src, - virStorageTypeToString(virStorageSourceGetActualType(src)), - src->path, - (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); - - if (VIR_ALLOC(priv) < 0) - return -1; - - src->drv->priv = priv; - - return 0; -} - - -static int -virStorageFileBackendFileCreate(virStorageSourcePtr src) -{ - int fd = -1; - mode_t mode = S_IRUSR; - - if (!src->readonly) - mode |= S_IWUSR; - - if ((fd = virFileOpenAs(src->path, O_WRONLY | O_TRUNC | O_CREAT, mode, - src->drv->uid, src->drv->gid, 0)) < 0) { - errno = -fd; - return -1; - } - - VIR_FORCE_CLOSE(fd); - return 0; -} - - -static int -virStorageFileBackendFileUnlink(virStorageSourcePtr src) -{ - return unlink(src->path); -} - - -static int -virStorageFileBackendFileStat(virStorageSourcePtr src, - struct stat *st) -{ - return stat(src->path, st); -} - - -static ssize_t -virStorageFileBackendFileRead(virStorageSourcePtr src, - size_t offset, - size_t len, - char **buf) -{ - int fd = -1; - ssize_t ret = -1; - - if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, - src->drv->uid, src->drv->gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), - src->path); - return -1; - } - - if (offset > 0) { - if (lseek(fd, offset, SEEK_SET) == (off_t) -1) { - virReportSystemError(errno, _("cannot seek into '%s'"), src->path); - goto cleanup; - } - } - - if ((ret = virFileReadHeaderFD(fd, len, buf)) < 0) { - virReportSystemError(errno, - _("cannot read header '%s'"), src->path); - goto cleanup; - } - - cleanup: - VIR_FORCE_CLOSE(fd); - - return ret; -} - - -static const char * -virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src) -{ - virStorageFileBackendFsPrivPtr priv = src->drv->priv; - - if (!priv->canonpath) { - if (!(priv->canonpath = canonicalize_file_name(src->path))) { - virReportSystemError(errno, _("can't canonicalize path '%s'"), - src->path); - return NULL; - } - } - - return priv->canonpath; -} - - -static int -virStorageFileBackendFileAccess(virStorageSourcePtr src, - int mode) -{ - return virFileAccessibleAs(src->path, mode, - src->drv->uid, src->drv->gid); -} - - -static int -virStorageFileBackendFileChown(const virStorageSource *src, - uid_t uid, - gid_t gid) -{ - return chown(src->path, uid, gid); -} - - -virStorageFileBackend virStorageFileBackendFile = { - .type = VIR_STORAGE_TYPE_FILE, - - .backendInit = virStorageFileBackendFileInit, - .backendDeinit = virStorageFileBackendFileDeinit, - - .storageFileCreate = virStorageFileBackendFileCreate, - .storageFileUnlink = virStorageFileBackendFileUnlink, - .storageFileStat = virStorageFileBackendFileStat, - .storageFileRead = virStorageFileBackendFileRead, - .storageFileAccess = virStorageFileBackendFileAccess, - .storageFileChown = virStorageFileBackendFileChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, -}; - - -virStorageFileBackend virStorageFileBackendBlock = { - .type = VIR_STORAGE_TYPE_BLOCK, - - .backendInit = virStorageFileBackendFileInit, - .backendDeinit = virStorageFileBackendFileDeinit, - - .storageFileStat = virStorageFileBackendFileStat, - .storageFileRead = virStorageFileBackendFileRead, - .storageFileAccess = virStorageFileBackendFileAccess, - .storageFileChown = virStorageFileBackendFileChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, -}; - - -virStorageFileBackend virStorageFileBackendDir = { - .type = VIR_STORAGE_TYPE_DIR, - - .backendInit = virStorageFileBackendFileInit, - .backendDeinit = virStorageFileBackendFileDeinit, - - .storageFileAccess = virStorageFileBackendFileAccess, - .storageFileChown = virStorageFileBackendFileChown, - - .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, -}; - - int virStorageBackendFsRegister(void) { @@ -911,13 +713,7 @@ virStorageBackendFsRegister(void) return -1; #endif /* WITH_STORAGE_FS */ - if (virStorageFileBackendRegister(&virStorageFileBackendFile) < 0) - return -1; - - if (virStorageFileBackendRegister(&virStorageFileBackendBlock) < 0) - return -1; - - if (virStorageFileBackendRegister(&virStorageFileBackendDir) < 0) + if (virStorageFileFsRegister() < 0) return -1; return 0; diff --git a/src/storage/storage_file_fs.c b/src/storage/storage_file_fs.c new file mode 100644 index 0000000000..c8d87514eb --- /dev/null +++ b/src/storage/storage_file_fs.c @@ -0,0 +1,251 @@ +/* + * storage_file_fs.c: storage file code for FS and directory handling + * + * Copyright (C) 2007-2018 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * 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: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <string.h> + +#include "virerror.h" +#include "storage_file_fs.h" +#include "storage_util.h" +#include "virstoragefilebackend.h" +#include "vircommand.h" +#include "viralloc.h" +#include "virfile.h" +#include "virlog.h" +#include "virstring.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +VIR_LOG_INIT("storage.storage_backend_fs"); + + +typedef struct _virStorageFileBackendFsPriv virStorageFileBackendFsPriv; +typedef virStorageFileBackendFsPriv *virStorageFileBackendFsPrivPtr; + +struct _virStorageFileBackendFsPriv { + char *canonpath; /* unique file identifier (canonical path) */ +}; + + +static void +virStorageFileBackendFileDeinit(virStorageSourcePtr src) +{ + VIR_DEBUG("deinitializing FS storage file %p (%s:%s)", src, + virStorageTypeToString(virStorageSourceGetActualType(src)), + src->path); + + virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + VIR_FREE(priv->canonpath); + VIR_FREE(priv); +} + + +static int +virStorageFileBackendFileInit(virStorageSourcePtr src) +{ + virStorageFileBackendFsPrivPtr priv = NULL; + + VIR_DEBUG("initializing FS storage file %p (%s:%s)[%u:%u]", src, + virStorageTypeToString(virStorageSourceGetActualType(src)), + src->path, + (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); + + if (VIR_ALLOC(priv) < 0) + return -1; + + src->drv->priv = priv; + + return 0; +} + + +static int +virStorageFileBackendFileCreate(virStorageSourcePtr src) +{ + int fd = -1; + mode_t mode = S_IRUSR; + + if (!src->readonly) + mode |= S_IWUSR; + + if ((fd = virFileOpenAs(src->path, O_WRONLY | O_TRUNC | O_CREAT, mode, + src->drv->uid, src->drv->gid, 0)) < 0) { + errno = -fd; + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + + +static int +virStorageFileBackendFileUnlink(virStorageSourcePtr src) +{ + return unlink(src->path); +} + + +static int +virStorageFileBackendFileStat(virStorageSourcePtr src, + struct stat *st) +{ + return stat(src->path, st); +} + + +static ssize_t +virStorageFileBackendFileRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) +{ + int fd = -1; + ssize_t ret = -1; + + if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, + src->drv->uid, src->drv->gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), + src->path); + return -1; + } + + if (offset > 0) { + if (lseek(fd, offset, SEEK_SET) == (off_t) -1) { + virReportSystemError(errno, _("cannot seek into '%s'"), src->path); + goto cleanup; + } + } + + if ((ret = virFileReadHeaderFD(fd, len, buf)) < 0) { + virReportSystemError(errno, + _("cannot read header '%s'"), src->path); + goto cleanup; + } + + cleanup: + VIR_FORCE_CLOSE(fd); + + return ret; +} + + +static const char * +virStorageFileBackendFileGetUniqueIdentifier(virStorageSourcePtr src) +{ + virStorageFileBackendFsPrivPtr priv = src->drv->priv; + + if (!priv->canonpath) { + if (!(priv->canonpath = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("can't canonicalize path '%s'"), + src->path); + return NULL; + } + } + + return priv->canonpath; +} + + +static int +virStorageFileBackendFileAccess(virStorageSourcePtr src, + int mode) +{ + return virFileAccessibleAs(src->path, mode, + src->drv->uid, src->drv->gid); +} + + +static int +virStorageFileBackendFileChown(const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + return chown(src->path, uid, gid); +} + + +virStorageFileBackend virStorageFileBackendFile = { + .type = VIR_STORAGE_TYPE_FILE, + + .backendInit = virStorageFileBackendFileInit, + .backendDeinit = virStorageFileBackendFileDeinit, + + .storageFileCreate = virStorageFileBackendFileCreate, + .storageFileUnlink = virStorageFileBackendFileUnlink, + .storageFileStat = virStorageFileBackendFileStat, + .storageFileRead = virStorageFileBackendFileRead, + .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, +}; + + +virStorageFileBackend virStorageFileBackendBlock = { + .type = VIR_STORAGE_TYPE_BLOCK, + + .backendInit = virStorageFileBackendFileInit, + .backendDeinit = virStorageFileBackendFileDeinit, + + .storageFileStat = virStorageFileBackendFileStat, + .storageFileRead = virStorageFileBackendFileRead, + .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, +}; + + +virStorageFileBackend virStorageFileBackendDir = { + .type = VIR_STORAGE_TYPE_DIR, + + .backendInit = virStorageFileBackendFileInit, + .backendDeinit = virStorageFileBackendFileDeinit, + + .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, + + .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, +}; + + +int +virStorageFileFsRegister(void) +{ + if (virStorageFileBackendRegister(&virStorageFileBackendFile) < 0) + return -1; + + if (virStorageFileBackendRegister(&virStorageFileBackendBlock) < 0) + return -1; + + if (virStorageFileBackendRegister(&virStorageFileBackendDir) < 0) + return -1; + + return 0; +} diff --git a/src/storage/storage_file_fs.h b/src/storage/storage_file_fs.h new file mode 100644 index 0000000000..c5d748c64d --- /dev/null +++ b/src/storage/storage_file_fs.h @@ -0,0 +1,29 @@ +/* + * storage_file_fs.h: storage file code for FS and directory handling + * + * Copyright (C) 2007-2018 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange + * + * 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: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __VIR_STORAGE_FILE_FS_H__ +# define __VIR_STORAGE_FILE_FS_H__ + +int virStorageFileFsRegister(void); + +#endif /* __VIR_STORAGE_FILE_FS_H__ */ -- 2.14.3

On Wed, Apr 25, 2018 at 16:52:40 +0100, Daniel Berrange wrote:
The storage file code needs to be run in the hypervisor drivers, while the storage backend code needs to be run in the storage driver. Split the source code as a preparatory step for creating separate loadable modules.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/Makefile.inc.am | 2 + src/storage/storage_backend_fs.c | 210 +------------------------------- src/storage/storage_file_fs.c | 251 +++++++++++++++++++++++++++++++++++++++ src/storage/storage_file_fs.h | 29 +++++ 4 files changed, 285 insertions(+), 207 deletions(-) create mode 100644 src/storage/storage_file_fs.c create mode 100644 src/storage/storage_file_fs.h
[...]
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 9b0fcf92c5..bface86b43 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2015 Red Hat, Inc. + * Copyright (C) 2007-2018 Red Hat, Inc.
Does just deleting a bunch of code warrant a copyright bump?
* Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -28,21 +28,15 @@ #include <stdio.h> #include <errno.h> #include <fcntl.h> -#include <unistd.h> #include <string.h>
-#include <libxml/parser.h> -#include <libxml/tree.h> -#include <libxml/xpath.h>
These are not added in the src/storage/storage_file_fs.c, so they are a separate cleanup.
- #include "virerror.h" #include "storage_backend_fs.h" +#include "storage_file_fs.h" #include "storage_util.h" #include "storage_conf.h" -#include "virstoragefilebackend.h" #include "vircommand.h" #include "viralloc.h" -#include "virxml.h" #include "virfile.h" #include "virlog.h" #include "virstring.h"
[...]
diff --git a/src/storage/storage_file_fs.c b/src/storage/storage_file_fs.c new file mode 100644 index 0000000000..c8d87514eb --- /dev/null +++ b/src/storage/storage_file_fs.c @@ -0,0 +1,251 @@ +/* + * storage_file_fs.c: storage file code for FS and directory handling + * + * Copyright (C) 2007-2018 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange
AFAIK there's no code moved which the last line would apply to.
+ * 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: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h>
[...]
diff --git a/src/storage/storage_file_fs.h b/src/storage/storage_file_fs.h new file mode 100644 index 0000000000..c5d748c64d --- /dev/null +++ b/src/storage/storage_file_fs.h @@ -0,0 +1,29 @@ +/* + * storage_file_fs.h: storage file code for FS and directory handling + * + * Copyright (C) 2007-2018 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange
As previously, the only added function is new in this commit.
+ * + * 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: Daniel P. Berrange <berrange@redhat.com> + */
ACK

On Thu, Apr 26, 2018 at 09:48:21AM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 16:52:40 +0100, Daniel Berrange wrote:
The storage file code needs to be run in the hypervisor drivers, while the storage backend code needs to be run in the storage driver. Split the source code as a preparatory step for creating separate loadable modules.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/storage/Makefile.inc.am | 2 + src/storage/storage_backend_fs.c | 210 +------------------------------- src/storage/storage_file_fs.c | 251 +++++++++++++++++++++++++++++++++++++++ src/storage/storage_file_fs.h | 29 +++++ 4 files changed, 285 insertions(+), 207 deletions(-) create mode 100644 src/storage/storage_file_fs.c create mode 100644 src/storage/storage_file_fs.h
[...]
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 9b0fcf92c5..bface86b43 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2015 Red Hat, Inc. + * Copyright (C) 2007-2018 Red Hat, Inc.
Does just deleting a bunch of code warrant a copyright bump?
Ok, will revert
* Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -28,21 +28,15 @@ #include <stdio.h> #include <errno.h> #include <fcntl.h> -#include <unistd.h> #include <string.h>
-#include <libxml/parser.h> -#include <libxml/tree.h> -#include <libxml/xpath.h>
These are not added in the src/storage/storage_file_fs.c, so they are a separate cleanup.
Yes, will split.
- #include "virerror.h" #include "storage_backend_fs.h" +#include "storage_file_fs.h" #include "storage_util.h" #include "storage_conf.h" -#include "virstoragefilebackend.h" #include "vircommand.h" #include "viralloc.h" -#include "virxml.h" #include "virfile.h" #include "virlog.h" #include "virstring.h"
[...]
diff --git a/src/storage/storage_file_fs.c b/src/storage/storage_file_fs.c new file mode 100644 index 0000000000..c8d87514eb --- /dev/null +++ b/src/storage/storage_file_fs.c @@ -0,0 +1,251 @@ +/* + * storage_file_fs.c: storage file code for FS and directory handling + * + * Copyright (C) 2007-2018 Red Hat, Inc. + * Copyright (C) 2007-2008 Daniel P. Berrange
AFAIK there's no code moved which the last line would apply to.
Yes, will change. 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 virStorageFileGetBackingStoreStr method has overloaded the NULL return value to indicate both no backing available and a fatal error dealing with it. The caller is thus not able to correctly propagate the error messages. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_driver.c | 15 +++++++++------ src/util/virstoragefile.c | 49 +++++++++++++++++++++++++++++++---------------- src/util/virstoragefile.h | 5 +++-- 3 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7484b00e23..672c5372eb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14608,12 +14608,15 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver, /* relative backing store paths need to be updated so that relative * block commit still works */ - if (reuse && - (backingStoreStr = virStorageFileGetBackingStoreStr(dd->src))) { - if (virStorageIsRelative(backingStoreStr)) - VIR_STEAL_PTR(dd->relPath, backingStoreStr); - else - VIR_FREE(backingStoreStr); + if (reuse) { + if (virStorageFileGetBackingStoreStr(dd->src, &backingStoreStr) < 0) + goto error; + if (backingStoreStr != NULL) { + if (virStorageIsRelative(backingStoreStr)) + VIR_STEAL_PTR(dd->relPath, backingStoreStr); + else + VIR_FREE(backingStoreStr); + } } /* Note that it's unsafe to assume that the disks in the persistent diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 531540ac91..f09035cd4a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4357,14 +4357,8 @@ virStorageFileRead(virStorageSourcePtr src, return -1; } - if (!src->drv->backend->storageFileRead) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("storage file reading is not supported for " - "storage type %s (protocol: %s)"), - virStorageTypeToString(src->type), - virStorageNetProtocolTypeToString(src->protocol)); + if (!src->drv->backend->storageFileRead) return -2; - } ret = src->drv->backend->storageFileRead(src, offset, len, buf); @@ -4551,8 +4545,15 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, goto cleanup; if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) + &buf)) < 0) { + if (headerLen == -2) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage file reading is not supported for " + "storage type %s (protocol: %s)"), + virStorageTypeToString(src->type), + virStorageNetProtocolTypeToString(src->protocol)); goto cleanup; + } if (virStorageFileGetMetadataInternal(src, buf, headerLen, &backingFormat) < 0) @@ -4664,24 +4665,36 @@ virStorageFileGetMetadata(virStorageSourcePtr src, * In case when the string can't be retrieved or does not exist NULL is * returned. */ -char * -virStorageFileGetBackingStoreStr(virStorageSourcePtr src) +int +virStorageFileGetBackingStoreStr(virStorageSourcePtr src, + char **backing) { virStorageSourcePtr tmp = NULL; char *buf = NULL; ssize_t headerLen; - char *ret = NULL; + int ret = -1; + int rv; + + *backing = NULL; /* exit if we can't load information about the current image */ if (!virStorageFileSupportsBackingChainTraversal(src)) - return NULL; + return 0; - if (virStorageFileAccess(src, F_OK) < 0) - return NULL; + rv = virStorageFileAccess(src, F_OK); + if (rv == -2) + return 0; + if (rv < 0) { + virStorageFileReportBrokenChain(errno, src, src); + return -1; + } if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) - return NULL; + &buf)) < 0) { + if (headerLen == -2) + return 0; + return -1; + } if (!(tmp = virStorageSourceCopy(src, false))) goto cleanup; @@ -4689,7 +4702,9 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src) if (virStorageFileGetMetadataInternal(tmp, buf, headerLen, NULL) < 0) goto cleanup; - VIR_STEAL_PTR(ret, tmp->backingStoreRaw); + VIR_STEAL_PTR(*backing, tmp->backingStoreRaw); + + ret = 0; cleanup: VIR_FREE(buf); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d129e81978..b92c1c47dd 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -474,8 +474,9 @@ int virStorageFileGetMetadata(virStorageSourcePtr src, bool report_broken) ATTRIBUTE_NONNULL(1); -char *virStorageFileGetBackingStoreStr(virStorageSourcePtr src) - ATTRIBUTE_NONNULL(1); +int virStorageFileGetBackingStoreStr(virStorageSourcePtr src, + char **backing) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); void virStorageFileReportBrokenChain(int errcode, virStorageSourcePtr src, -- 2.14.3

On Wed, Apr 25, 2018 at 16:52:41 +0100, Daniel Berrange wrote:
The virStorageFileGetBackingStoreStr method has overloaded the NULL return value to indicate both no backing available and a fatal error dealing with it.
The caller is thus not able to correctly propagate the error messages.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_driver.c | 15 +++++++++------ src/util/virstoragefile.c | 49 +++++++++++++++++++++++++++++++---------------- src/util/virstoragefile.h | 5 +++-- 3 files changed, 44 insertions(+), 25 deletions(-)
[...]
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 531540ac91..f09035cd4a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4357,14 +4357,8 @@ virStorageFileRead(virStorageSourcePtr src, return -1; }
- if (!src->drv->backend->storageFileRead) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("storage file reading is not supported for " - "storage type %s (protocol: %s)"), - virStorageTypeToString(src->type), - virStorageNetProtocolTypeToString(src->protocol)); + if (!src->drv->backend->storageFileRead)
This is called from qemuDomainBlockPeek, where this would cause no error to be reported. ACK with the above addressed.

On Thu, Apr 26, 2018 at 09:56:16AM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 16:52:41 +0100, Daniel Berrange wrote:
The virStorageFileGetBackingStoreStr method has overloaded the NULL return value to indicate both no backing available and a fatal error dealing with it.
The caller is thus not able to correctly propagate the error messages.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_driver.c | 15 +++++++++------ src/util/virstoragefile.c | 49 +++++++++++++++++++++++++++++++---------------- src/util/virstoragefile.h | 5 +++-- 3 files changed, 44 insertions(+), 25 deletions(-)
[...]
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 531540ac91..f09035cd4a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4357,14 +4357,8 @@ virStorageFileRead(virStorageSourcePtr src, return -1; }
- if (!src->drv->backend->storageFileRead) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("storage file reading is not supported for " - "storage type %s (protocol: %s)"), - virStorageTypeToString(src->type), - virStorageNetProtocolTypeToString(src->protocol)); + if (!src->drv->backend->storageFileRead)
This is called from qemuDomainBlockPeek, where this would cause no error to be reported.
Ok, will add the obvious error reporting fix.
ACK with the above addressed.
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 virStorageFileSupportsSecurityDriver and virStorageFileSupportsAccess currently just return a boolean value. This is ok because they don't have any failure scenarios but a subsequent patch is going to introduce potential failure scenario. This changes their return type from a boolean to an int with values -1, 0, 1. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_domain.c | 21 +++++++++------ src/qemu/qemu_driver.c | 6 +++-- src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++---------------- src/util/virstoragefile.h | 4 +-- 4 files changed, 63 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 326c939c85..542e20c5e4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7514,19 +7514,24 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, /* skip to the end of the chain if there is any */ while (virStorageSourceHasBacking(src)) { - if (report_broken && - virStorageFileSupportsAccess(src)) { + if (report_broken) { + int rv = virStorageFileSupportsAccess(src); - if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0) + if (rv < 0) goto cleanup; - if (virStorageFileAccess(src, F_OK) < 0) { - virStorageFileReportBrokenChain(errno, src, disk->src); + if (rv > 0) { + if (qemuDomainStorageFileInit(driver, vm, src, disk->src) < 0) + goto cleanup; + + if (virStorageFileAccess(src, F_OK) < 0) { + virStorageFileReportBrokenChain(errno, src, disk->src); + virStorageFileDeinit(src); + goto cleanup; + } + virStorageFileDeinit(src); - goto cleanup; } - - virStorageFileDeinit(src); } src = src->backingStore; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 672c5372eb..168a7c9ff3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -308,9 +308,11 @@ qemuSecurityChownCallback(const virStorageSource *src, struct stat sb; int save_errno = 0; int ret = -1; + int rv; - if (!virStorageFileSupportsSecurityDriver(src)) - return 0; + rv = virStorageFileSupportsSecurityDriver(src); + if (rv <= 0) + return rv; if (virStorageSourceIsLocalStorage(src)) { /* use direct chmod for local files so that the file doesn't diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f09035cd4a..da13d51d32 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4098,34 +4098,46 @@ virStorageFileIsInitialized(const virStorageSource *src) } -static virStorageFileBackendPtr -virStorageFileGetBackendForSupportCheck(const virStorageSource *src) +static int +virStorageFileGetBackendForSupportCheck(const virStorageSource *src, + virStorageFileBackendPtr *backend) { int actualType; - if (!src) - return NULL; - if (src->drv) - return src->drv->backend; + if (!src) { + *backend = NULL; + return 0; + } + + if (src->drv) { + *backend = src->drv->backend; + return 0; + } actualType = virStorageSourceGetActualType(src); - return virStorageFileBackendForTypeInternal(actualType, src->protocol, false); + *backend = virStorageFileBackendForTypeInternal(actualType, src->protocol, false); + return 0; } -static bool +static int virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) { virStorageFileBackendPtr backend; + int ret; - if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + + if (!backend) + return 0; return backend->storageFileGetUniqueIdentifier && - backend->storageFileRead && - backend->storageFileAccess; + backend->storageFileRead && + backend->storageFileAccess ? 1 : 0; } @@ -4137,15 +4149,19 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * Check if a storage file supports operations needed by the security * driver to perform labelling */ -bool +int virStorageFileSupportsSecurityDriver(const virStorageSource *src) { virStorageFileBackendPtr backend; + int ret; - if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + if (backend == NULL) + return 0; - return !!backend->storageFileChown; + return backend->storageFileChown ? 1 : 0; } @@ -4157,15 +4173,19 @@ virStorageFileSupportsSecurityDriver(const virStorageSource *src) * Check if a storage file supports checking if the storage source is accessible * for the given vm. */ -bool +int virStorageFileSupportsAccess(const virStorageSource *src) { virStorageFileBackendPtr backend; + int ret; - if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + if (backend == NULL) + return 0; - return !!backend->storageFileAccess; + return backend->storageFileAccess ? 1 : 0; } @@ -4514,14 +4534,16 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ssize_t headerLen; virStorageSourcePtr backingStore = NULL; int backingFormat; + int rv; VIR_DEBUG("path=%s format=%d uid=%u gid=%u probe=%d", src->path, src->format, (unsigned int)uid, (unsigned int)gid, allow_probe); /* exit if we can't load information about the current image */ - if (!virStorageFileSupportsBackingChainTraversal(src)) - return 0; + rv = virStorageFileSupportsBackingChainTraversal(src); + if (rv <= 0) + return rv; if (virStorageFileInitAs(src, uid, gid) < 0) return -1; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b92c1c47dd..0909fe212c 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -465,8 +465,8 @@ const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid); -bool virStorageFileSupportsSecurityDriver(const virStorageSource *src); -bool virStorageFileSupportsAccess(const virStorageSource *src); +int virStorageFileSupportsSecurityDriver(const virStorageSource *src); +int virStorageFileSupportsAccess(const virStorageSource *src); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, -- 2.14.3

On Wed, Apr 25, 2018 at 16:52:42 +0100, Daniel Berrange wrote:
The virStorageFileSupportsSecurityDriver and virStorageFileSupportsAccess currently just return a boolean value. This is ok because they don't have any failure scenarios but a subsequent patch is going to introduce potential failure scenario. This changes their return type from a boolean to an int with values -1, 0, 1.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_domain.c | 21 +++++++++------ src/qemu/qemu_driver.c | 6 +++-- src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++---------------- src/util/virstoragefile.h | 4 +-- 4 files changed, 63 insertions(+), 34 deletions(-)
[...]
index f09035cd4a..da13d51d32 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4098,34 +4098,46 @@ virStorageFileIsInitialized(const virStorageSource *src)
[...]
-static bool +static int virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) { virStorageFileBackendPtr backend; + int ret;
Hmm, isn't 'rv' better in the case when this variable actually is not returned?
- if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + + if (!backend) + return 0;
return backend->storageFileGetUniqueIdentifier && - backend->storageFileRead && - backend->storageFileAccess; + backend->storageFileRead && + backend->storageFileAccess ? 1 : 0;
Alignment looks off
}
@@ -4137,15 +4149,19 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * Check if a storage file supports operations needed by the security * driver to perform labelling */ -bool +int virStorageFileSupportsSecurityDriver(const virStorageSource *src) { virStorageFileBackendPtr backend; + int ret;
As in above hunk.
- if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + if (backend == NULL) + return 0;
- return !!backend->storageFileChown; + return backend->storageFileChown ? 1 : 0;
ACK

On Thu, Apr 26, 2018 at 11:15:42AM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 16:52:42 +0100, Daniel Berrange wrote:
The virStorageFileSupportsSecurityDriver and virStorageFileSupportsAccess currently just return a boolean value. This is ok because they don't have any failure scenarios but a subsequent patch is going to introduce potential failure scenario. This changes their return type from a boolean to an int with values -1, 0, 1.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_domain.c | 21 +++++++++------ src/qemu/qemu_driver.c | 6 +++-- src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++---------------- src/util/virstoragefile.h | 4 +-- 4 files changed, 63 insertions(+), 34 deletions(-)
[...]
index f09035cd4a..da13d51d32 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4098,34 +4098,46 @@ virStorageFileIsInitialized(const virStorageSource *src)
[...]
-static bool +static int virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) { virStorageFileBackendPtr backend; + int ret;
Hmm, isn't 'rv' better in the case when this variable actually is not returned?
Sure will change it.
- if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + + if (!backend) + return 0;
return backend->storageFileGetUniqueIdentifier && - backend->storageFileRead && - backend->storageFileAccess; + backend->storageFileRead && + backend->storageFileAccess ? 1 : 0;
Alignment looks off
Depends on your POV - this is standard indentation after a new line - it would only line up following lines if there was a opening round bracket on first line. That said I'll change it to avoid affecting pre-existing code alignment.
}
@@ -4137,15 +4149,19 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) * Check if a storage file supports operations needed by the security * driver to perform labelling */ -bool +int virStorageFileSupportsSecurityDriver(const virStorageSource *src) { virStorageFileBackendPtr backend; + int ret;
As in above hunk.
- if (!(backend = virStorageFileGetBackendForSupportCheck(src))) - return false; + ret = virStorageFileGetBackendForSupportCheck(src, &backend); + if (ret < 0) + return -1; + if (backend == NULL) + return 0;
- return !!backend->storageFileChown; + return backend->storageFileChown ? 1 : 0;
ACK
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 storage file drivers are currently loaded as a side effect of loading the storage driver. This is a bogus dependancy because the storage file code has no interaction with the storage drivers, and even ultimately be running in a completely separate daemon. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 4 ++ src/storage/Makefile.inc.am | 40 +++++++++++++++++++ src/storage/storage_backend_fs.c | 4 -- src/storage/storage_backend_gluster.c | 4 -- src/util/virstoragefile.c | 10 +++-- src/util/virstoragefilebackend.c | 72 +++++++++++++++++++++++++++-------- src/util/virstoragefilebackend.h | 9 ++--- 7 files changed, 111 insertions(+), 32 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 5090dfa2aa..e8705bbb48 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1384,6 +1384,8 @@ rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/connection-driver/*.la rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/connection-driver/*.a rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/storage-backend/*.la rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/storage-backend/*.a +rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/storage-file/*.la +rm -f $RPM_BUILD_ROOT%{_libdir}/libvirt/storage-file/*.a %if %{with_wireshark} rm -f $RPM_BUILD_ROOT%{_libdir}/wireshark/plugins/libvirt.la %endif @@ -1886,6 +1888,7 @@ exit 0 %attr(0755, root, root) %{_libexecdir}/libvirt_parthelper %{_libdir}/%{name}/connection-driver/libvirt_driver_storage.so %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_fs.so +%{_libdir}/%{name}/storage-file/libvirt_storage_file_fs.so %files daemon-driver-storage-disk %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_disk.so @@ -1905,6 +1908,7 @@ exit 0 %if %{with_storage_gluster} %files daemon-driver-storage-gluster %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_gluster.so +%{_libdir}/%{name}/storage-file/libvirt_storage_file_gluster.so %endif %if %{with_storage_rbd} diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am index af2c97ab93..ea98c0ee52 100644 --- a/src/storage/Makefile.inc.am +++ b/src/storage/Makefile.inc.am @@ -14,6 +14,9 @@ STORAGE_DRIVER_SOURCES = \ STORAGE_DRIVER_FS_SOURCES = \ storage/storage_backend_fs.h \ storage/storage_backend_fs.c \ + $(NULL) + +STORAGE_FILE_FS_SOURCES = \ storage/storage_file_fs.h \ storage/storage_file_fs.c \ $(NULL) @@ -57,6 +60,9 @@ STORAGE_DRIVER_SHEEPDOG_SOURCES = \ STORAGE_DRIVER_GLUSTER_SOURCES = \ storage/storage_backend_gluster.h \ storage/storage_backend_gluster.c \ + $(NULL) + +STORAGE_FILE_GLUSTER_SOURCES = \ storage/storage_file_gluster.h \ storage/storage_file_gluster.c \ $(NULL) @@ -80,6 +86,7 @@ STATEFUL_DRIVER_SOURCE_FILES += $(STORAGE_DRIVER_SOURCES) EXTRA_DIST += \ $(STORAGE_DRIVER_SOURCES) \ $(STORAGE_DRIVER_FS_SOURCES) \ + $(STORAGE_FILE_FS_SOURCES) \ $(STORAGE_DRIVER_LVM_SOURCES) \ $(STORAGE_DRIVER_ISCSI_SOURCES) \ $(STORAGE_DRIVER_SCSI_SOURCES) \ @@ -88,6 +95,7 @@ EXTRA_DIST += \ $(STORAGE_DRIVER_RBD_SOURCES) \ $(STORAGE_DRIVER_SHEEPDOG_SOURCES) \ $(STORAGE_DRIVER_GLUSTER_SOURCES) \ + $(STORAGE_FILE_GLUSTER_SOURCES) \ $(STORAGE_DRIVER_ZFS_SOURCES) \ $(STORAGE_DRIVER_VSTORAGE_SOURCES) \ $(STORAGE_HELPER_DISK_SOURCES) \ @@ -96,6 +104,9 @@ EXTRA_DIST += \ storagebackenddir = $(libdir)/libvirt/storage-backend storagebackend_LTLIBRARIES = +storagefiledir = $(libdir)/libvirt/storage-file +storagefile_LTLIBRARIES = + # Needed to keep automake quiet about conditionals libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ @@ -136,6 +147,19 @@ libvirt_storage_backend_fs_la_LIBADD = \ libvirt.la \ ../gnulib/lib/libgnu.la \ $(NULL) + +libvirt_storage_file_fs_la_SOURCES = $(STORAGE_FILE_FS_SOURCES) +libvirt_storage_file_fs_la_CFLAGS = \ + -I$(srcdir)/conf \ + $(AM_CFLAGS) \ + $(NULL) + +storagefile_LTLIBRARIES += libvirt_storage_file_fs.la +libvirt_storage_file_fs_la_LDFLAGS = $(AM_LDFLAGS_MOD) +libvirt_storage_file_fs_la_LIBADD = \ + libvirt.la \ + ../gnulib/lib/libgnu.la \ + $(NULL) endif WITH_STORAGE if WITH_STORAGE_LVM @@ -270,6 +294,22 @@ libvirt_storage_backend_gluster_la_CFLAGS = \ storagebackend_LTLIBRARIES += libvirt_storage_backend_gluster.la libvirt_storage_backend_gluster_la_LDFLAGS = $(AM_LDFLAGS_MOD) + + +libvirt_storage_file_gluster_la_SOURCES = $(STORAGE_FILE_GLUSTER_SOURCES) +libvirt_storage_file_gluster_la_LIBADD = \ + libvirt.la \ + $(GLUSTERFS_LIBS) \ + ../gnulib/lib/libgnu.la \ + $(NULL) +libvirt_storage_file_gluster_la_CFLAGS = \ + -I$(srcdir)/conf \ + $(GLUSTERFS_CFLAGS) \ + $(AM_CFLAGS) \ + $(NULL) + +storagefile_LTLIBRARIES += libvirt_storage_file_gluster.la +libvirt_storage_file_gluster_la_LDFLAGS = $(AM_LDFLAGS_MOD) endif WITH_STORAGE_GLUSTER if WITH_STORAGE_ZFS diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index bface86b43..37fea8c941 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -32,7 +32,6 @@ #include "virerror.h" #include "storage_backend_fs.h" -#include "storage_file_fs.h" #include "storage_util.h" #include "storage_conf.h" #include "vircommand.h" @@ -713,8 +712,5 @@ virStorageBackendFsRegister(void) return -1; #endif /* WITH_STORAGE_FS */ - if (virStorageFileFsRegister() < 0) - return -1; - return 0; } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index aca772676c..5d4b920a60 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -24,7 +24,6 @@ #include <glusterfs/api/glfs.h> #include "storage_backend_gluster.h" -#include "storage_file_gluster.h" #include "storage_conf.h" #include "viralloc.h" #include "virerror.h" @@ -567,8 +566,5 @@ virStorageBackendGlusterRegister(void) if (virStorageBackendRegister(&virStorageBackendGluster) < 0) return -1; - if (virStorageFileGlusterRegister() < 0) - return -1; - return 0; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index da13d51d32..73937b527a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4117,7 +4117,9 @@ virStorageFileGetBackendForSupportCheck(const virStorageSource *src, actualType = virStorageSourceGetActualType(src); - *backend = virStorageFileBackendForTypeInternal(actualType, src->protocol, false); + if (virStorageFileBackendForType(actualType, src->protocol, false, backend) < 0) + return -1; + return 0; } @@ -4234,8 +4236,10 @@ virStorageFileInitAs(virStorageSourcePtr src, else src->drv->gid = gid; - if (!(src->drv->backend = virStorageFileBackendForType(actualType, - src->protocol))) + if (virStorageFileBackendForType(actualType, + src->protocol, + true, + &src->drv->backend) < 0) goto error; if (src->drv->backend->backendInit && diff --git a/src/util/virstoragefilebackend.c b/src/util/virstoragefilebackend.c index df86ee39c2..ac087dabac 100644 --- a/src/util/virstoragefilebackend.c +++ b/src/util/virstoragefilebackend.c @@ -32,6 +32,7 @@ #include "internal.h" #include "virstoragefilebackend.h" #include "virlog.h" +#include "virmodule.h" #include "virfile.h" #include "configmake.h" @@ -44,6 +45,46 @@ VIR_LOG_INIT("storage.storage_source_backend"); static virStorageFileBackendPtr virStorageFileBackends[VIR_STORAGE_BACKENDS_MAX]; static size_t virStorageFileBackendsCount; +#define STORAGE_FILE_MODULE_DIR LIBDIR "/libvirt/storage-file" + +static int +virStorageFileLoadBackendModule(const char *name, + const char *regfunc, + bool forceload) +{ + char *modfile = NULL; + int ret; + + if (!(modfile = virFileFindResourceFull(name, + "libvirt_storage_file_", + ".so", + abs_topbuilddir "/src/.libs", + STORAGE_FILE_MODULE_DIR, + "LIBVIRT_STORAGE_FILE_DIR"))) + return -1; + + ret = virModuleLoad(modfile, regfunc, forceload); + + VIR_FREE(modfile); + + return ret; +} + + +static int virStorageFileBackendOnceInit(void) +{ +#if WITH_STORAGE_DIR || WITH_STORAGE_FS + if (virStorageFileLoadBackendModule("fs", "virStorageFileFsRegister", false) < 0) + return -1; +#endif /* WITH_STORAGE_DIR || WITH_STORAGE_FS */ +#if WITH_STORAGE_GLUSTER + if (virStorageFileLoadBackendModule("gluster", "virStorageFileGlusterRegister", false) < 0) + return -1; +#endif /* WITH_STORAGE_GLUSTER */ + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virStorageFileBackend) int virStorageFileBackendRegister(virStorageFileBackendPtr backend) @@ -65,25 +106,32 @@ virStorageFileBackendRegister(virStorageFileBackendPtr backend) return 0; } -virStorageFileBackendPtr -virStorageFileBackendForTypeInternal(int type, - int protocol, - bool report) +int +virStorageFileBackendForType(int type, + int protocol, + bool required, + virStorageFileBackendPtr *backend) { size_t i; + *backend = NULL; + + if (virStorageFileBackendInitialize() < 0) + return -1; + for (i = 0; i < virStorageFileBackendsCount; i++) { if (virStorageFileBackends[i]->type == type) { if (type == VIR_STORAGE_TYPE_NETWORK && virStorageFileBackends[i]->protocol != protocol) continue; - return virStorageFileBackends[i]; + *backend = virStorageFileBackends[i]; + return 0; } } - if (!report) - return NULL; + if (!required) + return 0; if (type == VIR_STORAGE_TYPE_NETWORK) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -96,13 +144,5 @@ virStorageFileBackendForTypeInternal(int type, virStorageTypeToString(type)); } - return NULL; -} - - -virStorageFileBackendPtr -virStorageFileBackendForType(int type, - int protocol) -{ - return virStorageFileBackendForTypeInternal(type, protocol, true); + return -1; } diff --git a/src/util/virstoragefilebackend.h b/src/util/virstoragefilebackend.h index 6cd51750ee..6686864367 100644 --- a/src/util/virstoragefilebackend.h +++ b/src/util/virstoragefilebackend.h @@ -71,11 +71,10 @@ typedef int uid_t uid, gid_t gid); -virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); -virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type, - int protocol, - bool report); - +int virStorageFileBackendForType(int type, + int protocol, + bool required, + virStorageFileBackendPtr *backend); struct _virStorageFileBackend { int type; -- 2.14.3

On Wed, Apr 25, 2018 at 16:52:43 +0100, Daniel Berrange wrote:
The storage file drivers are currently loaded as a side effect of loading the storage driver. This is a bogus dependancy because the storage file code has no interaction with the storage drivers, and even ultimately be running in a completely separate daemon.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 4 ++ src/storage/Makefile.inc.am | 40 +++++++++++++++++++ src/storage/storage_backend_fs.c | 4 -- src/storage/storage_backend_gluster.c | 4 -- src/util/virstoragefile.c | 10 +++-- src/util/virstoragefilebackend.c | 72 +++++++++++++++++++++++++++-------- src/util/virstoragefilebackend.h | 9 ++--- 7 files changed, 111 insertions(+), 32 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 5090dfa2aa..e8705bbb48 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in
[...]
@@ -1886,6 +1888,7 @@ exit 0 %attr(0755, root, root) %{_libexecdir}/libvirt_parthelper %{_libdir}/%{name}/connection-driver/libvirt_driver_storage.so %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_fs.so +%{_libdir}/%{name}/storage-file/libvirt_storage_file_fs.so
%files daemon-driver-storage-disk %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_disk.so @@ -1905,6 +1908,7 @@ exit 0 %if %{with_storage_gluster} %files daemon-driver-storage-gluster %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_gluster.so +%{_libdir}/%{name}/storage-file/libvirt_storage_file_gluster.so
Should we also later create a few sub-packages so that the qemu driver can be installed without the storage driver? ACK, the code looks good, but I'm running 'make rpm' and 'make dist' which will take a while. I'll report only if something breaks.

On Thu, Apr 26, 2018 at 10:52:02AM +0200, Peter Krempa wrote:
On Wed, Apr 25, 2018 at 16:52:43 +0100, Daniel Berrange wrote:
The storage file drivers are currently loaded as a side effect of loading the storage driver. This is a bogus dependancy because the storage file code has no interaction with the storage drivers, and even ultimately be running in a completely separate daemon.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- libvirt.spec.in | 4 ++ src/storage/Makefile.inc.am | 40 +++++++++++++++++++ src/storage/storage_backend_fs.c | 4 -- src/storage/storage_backend_gluster.c | 4 -- src/util/virstoragefile.c | 10 +++-- src/util/virstoragefilebackend.c | 72 +++++++++++++++++++++++++++-------- src/util/virstoragefilebackend.h | 9 ++--- 7 files changed, 111 insertions(+), 32 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in index 5090dfa2aa..e8705bbb48 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in
[...]
@@ -1886,6 +1888,7 @@ exit 0 %attr(0755, root, root) %{_libexecdir}/libvirt_parthelper %{_libdir}/%{name}/connection-driver/libvirt_driver_storage.so %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_fs.so +%{_libdir}/%{name}/storage-file/libvirt_storage_file_fs.so
%files daemon-driver-storage-disk %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_disk.so @@ -1905,6 +1908,7 @@ exit 0 %if %{with_storage_gluster} %files daemon-driver-storage-gluster %{_libdir}/%{name}/storage-backend/libvirt_storage_backend_gluster.so +%{_libdir}/%{name}/storage-file/libvirt_storage_file_gluster.so
Should we also later create a few sub-packages so that the qemu driver can be installed without the storage driver?
Yeah, it is probably the right thing todo since we try to make each loadable module independent.
ACK, the code looks good, but I'm running 'make rpm' and 'make dist' which will take a while. I'll report only if something breaks.
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 :|
participants (2)
-
Daniel P. Berrangé
-
Peter Krempa