[libvirt] [PATCH 0/6] qemu: Clean up storage file access
A collection of few cleanups and bugfixes. Peter Krempa (6): lib: Remove misplaced and redundant comments tests: storage: Fully register storage driver storage: Split out virStorageSource accessors to separate file storage: Make virStorageFileReadHeader more universal qemu: Use storage driver APIs in qemuDomainBlockPeek qemu: Support only raw volumes in qemuDomainBlockPeek po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt-domain.c | 4 +- src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 35 +- src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend.h | 9 +- src/storage/storage_backend_fs.c | 20 +- src/storage/storage_backend_gluster.c | 37 ++- src/storage/storage_driver.c | 551 +------------------------------ src/storage/storage_driver.h | 28 -- src/storage/storage_source.c | 587 ++++++++++++++++++++++++++++++++++ src/storage/storage_source.h | 54 ++++ tests/virstoragetest.c | 6 +- 14 files changed, 709 insertions(+), 627 deletions(-) create mode 100644 src/storage/storage_source.c create mode 100644 src/storage/storage_source.h -- 2.12.2
It's obvious that unsigned long long is 64 bit and also our web page generator would misplace the comment after the return value due to the way it's parsing them. --- src/libvirt-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 9bda3c204..4033ae8e6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -5820,7 +5820,7 @@ virDomainMemoryStats(virDomainPtr dom, virDomainMemoryStatPtr stats, int virDomainBlockPeek(virDomainPtr dom, const char *disk, - unsigned long long offset /* really 64 bits */, + unsigned long long offset, size_t size, void *buffer, unsigned int flags) @@ -5956,7 +5956,7 @@ virDomainBlockResize(virDomainPtr dom, */ int virDomainMemoryPeek(virDomainPtr dom, - unsigned long long start /* really 64 bits */, + unsigned long long start, size_t size, void *buffer, unsigned int flags) -- 2.12.2
On 06/23/2017 09:33 AM, Peter Krempa wrote:
It's obvious that unsigned long long is 64 bit and also our web page generator would misplace the comment after the return value due to the way it's parsing them. --- src/libvirt-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John (this one's safe for freeze if you care, but the rest should wait for after release)
On Sat, Jul 01, 2017 at 10:41:24 -0400, John Ferlan wrote:
On 06/23/2017 09:33 AM, Peter Krempa wrote:
It's obvious that unsigned long long is 64 bit and also our web page generator would misplace the comment after the return value due to the way it's parsing them. --- src/libvirt-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
(this one's safe for freeze if you care, but the rest should wait for after release)
I've pushed this one so that the docs embedded in the upcomming release are already fixed. Thanks.
Use the full storage driver registration method that also fails if one of the storage backends is not present. This makes the test fail if a submodule fails registration, which is useful for testing. Additionally return EXIT_FAILURE as usual in tests rather than -1. --- tests/virstoragetest.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f34408395..a73c53839 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -32,7 +32,6 @@ #include "dirname.h" #include "storage/storage_driver.h" -#include "storage/storage_backend.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -732,8 +731,8 @@ mymain(void) virStorageSourcePtr chain2; /* short for chain->backingStore */ virStorageSourcePtr chain3; /* short for chain2->backingStore */ - if (virStorageBackendDriversRegister(false) < 0) - return -1; + if (storageRegisterAll() < 0) + return EXIT_FAILURE; /* Prep some files with qemu-img; if that is not found on PATH, or * if it lacks support for qcow2 and qed, skip this test. */ -- 2.12.2
On 06/23/2017 09:33 AM, Peter Krempa wrote:
Use the full storage driver registration method that also fails if one of the storage backends is not present. This makes the test fail if a submodule fails registration, which is useful for testing.
Additionally return EXIT_FAILURE as usual in tests rather than -1. --- tests/virstoragetest.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The helper methods for actually accessing the storage objects don't really belong to the main storage driver implementation file. Split them out. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 1 + src/security/virt-aa-helper.c | 2 +- src/storage/storage_driver.c | 551 +-------------------------------------- src/storage/storage_driver.h | 28 -- src/storage/storage_source.c | 585 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_source.h | 53 ++++ tests/virstoragetest.c | 1 + 10 files changed, 645 insertions(+), 579 deletions(-) create mode 100644 src/storage/storage_source.c create mode 100644 src/storage/storage_source.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f29..87634d228 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -185,6 +185,7 @@ src/storage/storage_backend_sheepdog.c src/storage/storage_backend_vstorage.c src/storage/storage_backend_zfs.c src/storage/storage_driver.c +src/storage/storage_source.c src/storage/storage_util.c src/test/test_driver.c src/uml/uml_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index eae32dc58..399d031dd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1053,6 +1053,7 @@ STORAGE_DRIVER_BACKEND_SOURCES = \ STORAGE_DRIVER_SOURCES = \ storage/storage_driver.h storage/storage_driver.c \ + storage/storage_source.h storage/storage_source.c \ $(STORAGE_DRIVER_BACKEND_SOURCES) \ storage/storage_util.h storage/storage_util.c diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e7404da6..bc3a85ca2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -54,6 +54,7 @@ #include "locking/domain_lock.h" #include "storage/storage_driver.h" +#include "storage/storage_source.h" #ifdef MAJOR_IN_MKDEV # include <sys/mkdev.h> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e91663ca9..052c7f0fc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -100,6 +100,7 @@ #include "viraccessapicheck.h" #include "viraccessapicheckqemu.h" #include "storage/storage_driver.h" +#include "storage/storage_source.h" #include "virhostdev.h" #include "domain_capabilities.h" #include "vircgroup.h" diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 7f3b7ad08..695272076 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,7 +55,7 @@ #include "virstring.h" #include "virgettext.h" -#include "storage/storage_driver.h" +#include "storage/storage_source.h" #define VIR_FROM_THIS VIR_FROM_SECURITY diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ab1dc8f36..ffffd5a37 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -50,7 +50,7 @@ #include "configmake.h" #include "virstring.h" #include "viraccessapicheck.h" -#include "dirname.h" +//#include "dirname.h" #include "storage_util.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -2758,555 +2758,6 @@ storageRegisterAll(void) } -/* ----------- file handlers cooperating with storage driver --------------- */ -static bool -virStorageFileIsInitialized(const virStorageSource *src) -{ - return src && src->drv; -} - - -static bool -virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) -{ - int actualType; - virStorageFileBackendPtr backend; - - if (!src) - return false; - actualType = virStorageSourceGetActualType(src); - - if (src->drv) { - backend = src->drv->backend; - } else { - if (!(backend = virStorageFileBackendForTypeInternal(actualType, - src->protocol, - false))) - return false; - } - - return backend->storageFileGetUniqueIdentifier && - backend->storageFileReadHeader && - backend->storageFileAccess; -} - - -/** - * virStorageFileSupportsSecurityDriver: - * - * @src: a storage file structure - * - * Check if a storage file supports operations needed by the security - * driver to perform labelling - */ -bool -virStorageFileSupportsSecurityDriver(const virStorageSource *src) -{ - int actualType; - virStorageFileBackendPtr backend; - - if (!src) - return false; - actualType = virStorageSourceGetActualType(src); - - if (src->drv) { - backend = src->drv->backend; - } else { - if (!(backend = virStorageFileBackendForTypeInternal(actualType, - src->protocol, - false))) - return false; - } - - return !!backend->storageFileChown; -} - - -void -virStorageFileDeinit(virStorageSourcePtr src) -{ - if (!virStorageFileIsInitialized(src)) - return; - - if (src->drv->backend && - src->drv->backend->backendDeinit) - src->drv->backend->backendDeinit(src); - - VIR_FREE(src->drv); -} - - -/** - * virStorageFileInitAs: - * - * @src: storage source definition - * @uid: uid used to access the file, or -1 for current uid - * @gid: gid used to access the file, or -1 for current gid - * - * Initialize a storage source to be used with storage driver. Use the provided - * uid and gid if possible for the operations. - * - * Returns 0 if the storage file was successfully initialized, -1 if the - * initialization failed. Libvirt error is reported. - */ -int -virStorageFileInitAs(virStorageSourcePtr src, - uid_t uid, gid_t gid) -{ - int actualType = virStorageSourceGetActualType(src); - if (VIR_ALLOC(src->drv) < 0) - return -1; - - if (uid == (uid_t) -1) - src->drv->uid = geteuid(); - else - src->drv->uid = uid; - - if (gid == (gid_t) -1) - src->drv->gid = getegid(); - else - src->drv->gid = gid; - - if (!(src->drv->backend = virStorageFileBackendForType(actualType, - src->protocol))) - goto error; - - if (src->drv->backend->backendInit && - src->drv->backend->backendInit(src) < 0) - goto error; - - return 0; - - error: - VIR_FREE(src->drv); - return -1; -} - - -/** - * virStorageFileInit: - * - * See virStorageFileInitAs. The file is initialized to be accessed by the - * current user. - */ -int -virStorageFileInit(virStorageSourcePtr src) -{ - return virStorageFileInitAs(src, -1, -1); -} - - -/** - * virStorageFileCreate: Creates an empty storage file via storage driver - * - * @src: file structure pointing to the file - * - * Returns 0 on success, -2 if the function isn't supported by the backend, - * -1 on other failure. Errno is set in case of failure. - */ -int -virStorageFileCreate(virStorageSourcePtr src) -{ - int ret; - - if (!virStorageFileIsInitialized(src) || - !src->drv->backend->storageFileCreate) { - errno = ENOSYS; - return -2; - } - - ret = src->drv->backend->storageFileCreate(src); - - VIR_DEBUG("created storage file %p: ret=%d, errno=%d", - src, ret, errno); - - return ret; -} - - -/** - * virStorageFileUnlink: Unlink storage file via storage driver - * - * @src: file structure pointing to the file - * - * Unlinks the file described by the @file structure. - * - * Returns 0 on success, -2 if the function isn't supported by the backend, - * -1 on other failure. Errno is set in case of failure. - */ -int -virStorageFileUnlink(virStorageSourcePtr src) -{ - int ret; - - if (!virStorageFileIsInitialized(src) || - !src->drv->backend->storageFileUnlink) { - errno = ENOSYS; - return -2; - } - - ret = src->drv->backend->storageFileUnlink(src); - - VIR_DEBUG("unlinked storage file %p: ret=%d, errno=%d", - src, ret, errno); - - return ret; -} - - -/** - * virStorageFileStat: returns stat struct of a file via storage driver - * - * @src: file structure pointing to the file - * @stat: stat structure to return data - * - * Returns 0 on success, -2 if the function isn't supported by the backend, - * -1 on other failure. Errno is set in case of failure. -*/ -int -virStorageFileStat(virStorageSourcePtr src, - struct stat *st) -{ - int ret; - - if (!virStorageFileIsInitialized(src) || - !src->drv->backend->storageFileStat) { - errno = ENOSYS; - return -2; - } - - ret = src->drv->backend->storageFileStat(src, st); - - VIR_DEBUG("stat of storage file %p: ret=%d, errno=%d", - src, ret, errno); - - return ret; -} - - -/** - * virStorageFileReadHeader: read the beginning bytes of a file into a buffer - * - * @src: file structure pointing to the file - * @max_len: maximum number of bytes read from the storage file - * @buf: buffer to read the data into. buffer shall be freed by caller) - * - * Returns the count of bytes read on success and -1 on failure, -2 if the - * function isn't supported by the backend. - * Libvirt error is reported on failure. - */ -ssize_t -virStorageFileReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf) -{ - ssize_t ret; - - if (!virStorageFileIsInitialized(src)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("storage file backend not initialized")); - return -1; - } - - if (!src->drv->backend->storageFileReadHeader) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("storage file header reading is not supported for " - "storage type %s (protocol: %s)"), - virStorageTypeToString(src->type), - virStorageNetProtocolTypeToString(src->protocol)); - return -2; - } - - ret = src->drv->backend->storageFileReadHeader(src, max_len, buf); - - VIR_DEBUG("read of storage header %p: ret=%zd", src, ret); - - return ret; -} - - -/* - * virStorageFileGetUniqueIdentifier: Get a unique string describing the volume - * - * @src: file structure pointing to the file - * - * Returns a string uniquely describing a single volume (canonical path). - * The string shall not be freed and is valid until the storage file is - * deinitialized. Returns NULL on error and sets a libvirt error code */ -const char * -virStorageFileGetUniqueIdentifier(virStorageSourcePtr src) -{ - if (!virStorageFileIsInitialized(src)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("storage file backend not initialized")); - return NULL; - } - - if (!src->drv->backend->storageFileGetUniqueIdentifier) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unique storage file identifier not implemented for " - "storage type %s (protocol: %s)'"), - virStorageTypeToString(src->type), - virStorageNetProtocolTypeToString(src->protocol)); - return NULL; - } - - return src->drv->backend->storageFileGetUniqueIdentifier(src); -} - - -/** - * virStorageFileAccess: Check accessibility of a storage file - * - * @src: storage file to check access permissions - * @mode: accessibility check options (see man 2 access) - * - * Returns 0 on success, -1 on error and sets errno. No libvirt - * error is reported. Returns -2 if the operation isn't supported - * by libvirt storage backend. - */ -int -virStorageFileAccess(virStorageSourcePtr src, - int mode) -{ - if (!virStorageFileIsInitialized(src) || - !src->drv->backend->storageFileAccess) { - errno = ENOSYS; - return -2; - } - - return src->drv->backend->storageFileAccess(src, mode); -} - - -/** - * virStorageFileChown: Change owner of a storage file - * - * @src: storage file to change owner of - * @uid: new owner id - * @gid: new group id - * - * Returns 0 on success, -1 on error and sets errno. No libvirt - * error is reported. Returns -2 if the operation isn't supported - * by libvirt storage backend. - */ -int -virStorageFileChown(const virStorageSource *src, - uid_t uid, - gid_t gid) -{ - if (!virStorageFileIsInitialized(src) || - !src->drv->backend->storageFileChown) { - errno = ENOSYS; - return -2; - } - - VIR_DEBUG("chown of storage file %p to %u:%u", - src, (unsigned int)uid, (unsigned int)gid); - - return src->drv->backend->storageFileChown(src, uid, gid); -} - - -/* Recursive workhorse for virStorageFileGetMetadata. */ -static int -virStorageFileGetMetadataRecurse(virStorageSourcePtr src, - virStorageSourcePtr parent, - uid_t uid, gid_t gid, - bool allow_probe, - bool report_broken, - virHashTablePtr cycle) -{ - int ret = -1; - const char *uniqueName; - char *buf = NULL; - ssize_t headerLen; - virStorageSourcePtr backingStore = NULL; - int backingFormat; - - 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; - - if (virStorageFileInitAs(src, uid, gid) < 0) - return -1; - - if (virStorageFileAccess(src, F_OK) < 0) { - if (src == parent) { - virReportSystemError(errno, - _("Cannot access storage file '%s' " - "(as uid:%u, gid:%u)"), - src->path, (unsigned int)uid, - (unsigned int)gid); - } else { - virReportSystemError(errno, - _("Cannot access backing file '%s' " - "of storage file '%s' (as uid:%u, gid:%u)"), - src->path, parent->path, - (unsigned int)uid, (unsigned int)gid); - } - - goto cleanup; - } - - if (!(uniqueName = virStorageFileGetUniqueIdentifier(src))) - goto cleanup; - - if (virHashLookup(cycle, uniqueName)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s (%s) is self-referential"), - src->path, uniqueName); - goto cleanup; - } - - if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0) - goto cleanup; - - if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) - goto cleanup; - - if (virStorageFileGetMetadataInternal(src, buf, headerLen, - &backingFormat) < 0) - goto cleanup; - - /* check whether we need to go deeper */ - if (!src->backingStoreRaw) { - ret = 0; - goto cleanup; - } - - if (!(backingStore = virStorageSourceNewFromBacking(src))) - goto cleanup; - - if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) - backingStore->format = VIR_STORAGE_FILE_RAW; - else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) - backingStore->format = VIR_STORAGE_FILE_AUTO; - else - backingStore->format = backingFormat; - - if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent, - uid, gid, - allow_probe, report_broken, - cycle)) < 0) { - if (report_broken) - goto cleanup; - - /* if we fail somewhere midway, just accept and return a - * broken chain */ - ret = 0; - goto cleanup; - } - - src->backingStore = backingStore; - backingStore = NULL; - ret = 0; - - cleanup: - VIR_FREE(buf); - virStorageFileDeinit(src); - virStorageSourceFree(backingStore); - return ret; -} - - -/** - * virStorageFileGetMetadata: - * - * Extract metadata about the storage volume with the specified - * image format. If image format is VIR_STORAGE_FILE_AUTO, it - * will probe to automatically identify the format. Recurses through - * the entire chain. - * - * Open files using UID and GID (or pass -1 for the current user/group). - * Treat any backing files without explicit type as raw, unless ALLOW_PROBE. - * - * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a - * format, since a malicious guest can turn a raw file into any - * other non-raw format at will. - * - * If @report_broken is true, the whole function fails with a possibly sane - * error instead of just returning a broken chain. - * - * Caller MUST free result after use via virStorageSourceFree. - */ -int -virStorageFileGetMetadata(virStorageSourcePtr src, - uid_t uid, gid_t gid, - bool allow_probe, - bool report_broken) -{ - VIR_DEBUG("path=%s format=%d uid=%u gid=%u probe=%d, report_broken=%d", - src->path, src->format, (unsigned int)uid, (unsigned int)gid, - allow_probe, report_broken); - - virHashTablePtr cycle = NULL; - int ret = -1; - - if (!(cycle = virHashCreate(5, NULL))) - return -1; - - if (src->format <= VIR_STORAGE_FILE_NONE) - src->format = allow_probe ? - VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - - ret = virStorageFileGetMetadataRecurse(src, src, uid, gid, - allow_probe, report_broken, cycle); - - virHashFree(cycle); - return ret; -} - - -/** - * virStorageFileGetBackingStoreStr: - * @src: storage object - * - * Extracts the backing store string as stored in the storage volume described - * by @src and returns it to the user. Caller is responsible for freeing it. - * In case when the string can't be retrieved or does not exist NULL is - * returned. - */ -char * -virStorageFileGetBackingStoreStr(virStorageSourcePtr src) -{ - virStorageSourcePtr tmp = NULL; - char *buf = NULL; - ssize_t headerLen; - char *ret = NULL; - - /* exit if we can't load information about the current image */ - if (!virStorageFileSupportsBackingChainTraversal(src)) - return NULL; - - if (virStorageFileAccess(src, F_OK) < 0) - return NULL; - - if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) - return NULL; - - if (!(tmp = virStorageSourceCopy(src, false))) - goto cleanup; - - if (virStorageFileGetMetadataInternal(tmp, buf, headerLen, NULL) < 0) - goto cleanup; - - VIR_STEAL_PTR(ret, tmp->backingStoreRaw); - - cleanup: - VIR_FREE(buf); - virStorageSourceFree(tmp); - - return ret; -} - - static int virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, virStoragePoolDefPtr pooldef) diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index ade05f715..8b913a1b8 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -28,34 +28,6 @@ # include "domain_conf.h" # include "virstorageobj.h" -# include "virstoragefile.h" - -int virStorageFileInit(virStorageSourcePtr src); -int virStorageFileInitAs(virStorageSourcePtr src, - uid_t uid, gid_t gid); -void virStorageFileDeinit(virStorageSourcePtr src); - -int virStorageFileCreate(virStorageSourcePtr src); -int virStorageFileUnlink(virStorageSourcePtr src); -int virStorageFileStat(virStorageSourcePtr src, - struct stat *stat); -ssize_t virStorageFileReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf); -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); - -int virStorageFileGetMetadata(virStorageSourcePtr src, - uid_t uid, gid_t gid, - bool allow_probe, - bool report_broken) - ATTRIBUTE_NONNULL(1); - -char *virStorageFileGetBackingStoreStr(virStorageSourcePtr src) - ATTRIBUTE_NONNULL(1); int virStorageTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def); diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c new file mode 100644 index 000000000..c91d629c8 --- /dev/null +++ b/src/storage/storage_source.c @@ -0,0 +1,585 @@ +/* + * storage_source.c: Storage source object accessors to real storage + * + * 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 <sys/types.h> +#include <sys/stat.h> + +#include <errno.h> +#include <string.h> + +#include "virerror.h" +#include "storage_source.h" +#include "storage_backend.h" +#include "viralloc.h" +#include "virlog.h" +#include "virstring.h" +#include "virhash.h" + +#define VIR_FROM_THIS VIR_FROM_STORAGE + +VIR_LOG_INIT("storage.storage_source"); + + +static bool +virStorageFileIsInitialized(const virStorageSource *src) +{ + return src && src->drv; +} + + +static bool +virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) +{ + int actualType; + virStorageFileBackendPtr backend; + + if (!src) + return false; + actualType = virStorageSourceGetActualType(src); + + if (src->drv) { + backend = src->drv->backend; + } else { + if (!(backend = virStorageFileBackendForTypeInternal(actualType, + src->protocol, + false))) + return false; + } + + return backend->storageFileGetUniqueIdentifier && + backend->storageFileReadHeader && + backend->storageFileAccess; +} + + +/** + * virStorageFileSupportsSecurityDriver: + * + * @src: a storage file structure + * + * Check if a storage file supports operations needed by the security + * driver to perform labelling + */ +bool +virStorageFileSupportsSecurityDriver(const virStorageSource *src) +{ + int actualType; + virStorageFileBackendPtr backend; + + if (!src) + return false; + actualType = virStorageSourceGetActualType(src); + + if (src->drv) { + backend = src->drv->backend; + } else { + if (!(backend = virStorageFileBackendForTypeInternal(actualType, + src->protocol, + false))) + return false; + } + + return !!backend->storageFileChown; +} + + +void +virStorageFileDeinit(virStorageSourcePtr src) +{ + if (!virStorageFileIsInitialized(src)) + return; + + if (src->drv->backend && + src->drv->backend->backendDeinit) + src->drv->backend->backendDeinit(src); + + VIR_FREE(src->drv); +} + + +/** + * virStorageFileInitAs: + * + * @src: storage source definition + * @uid: uid used to access the file, or -1 for current uid + * @gid: gid used to access the file, or -1 for current gid + * + * Initialize a storage source to be used with storage driver. Use the provided + * uid and gid if possible for the operations. + * + * Returns 0 if the storage file was successfully initialized, -1 if the + * initialization failed. Libvirt error is reported. + */ +int +virStorageFileInitAs(virStorageSourcePtr src, + uid_t uid, gid_t gid) +{ + int actualType = virStorageSourceGetActualType(src); + if (VIR_ALLOC(src->drv) < 0) + return -1; + + if (uid == (uid_t) -1) + src->drv->uid = geteuid(); + else + src->drv->uid = uid; + + if (gid == (gid_t) -1) + src->drv->gid = getegid(); + else + src->drv->gid = gid; + + if (!(src->drv->backend = virStorageFileBackendForType(actualType, + src->protocol))) + goto error; + + if (src->drv->backend->backendInit && + src->drv->backend->backendInit(src) < 0) + goto error; + + return 0; + + error: + VIR_FREE(src->drv); + return -1; +} + + +/** + * virStorageFileInit: + * + * See virStorageFileInitAs. The file is initialized to be accessed by the + * current user. + */ +int +virStorageFileInit(virStorageSourcePtr src) +{ + return virStorageFileInitAs(src, -1, -1); +} + + +/** + * virStorageFileCreate: Creates an empty storage file via storage driver + * + * @src: file structure pointing to the file + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. + */ +int +virStorageFileCreate(virStorageSourcePtr src) +{ + int ret; + + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileCreate) { + errno = ENOSYS; + return -2; + } + + ret = src->drv->backend->storageFileCreate(src); + + VIR_DEBUG("created storage file %p: ret=%d, errno=%d", + src, ret, errno); + + return ret; +} + + +/** + * virStorageFileUnlink: Unlink storage file via storage driver + * + * @src: file structure pointing to the file + * + * Unlinks the file described by the @file structure. + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. + */ +int +virStorageFileUnlink(virStorageSourcePtr src) +{ + int ret; + + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileUnlink) { + errno = ENOSYS; + return -2; + } + + ret = src->drv->backend->storageFileUnlink(src); + + VIR_DEBUG("unlinked storage file %p: ret=%d, errno=%d", + src, ret, errno); + + return ret; +} + + +/** + * virStorageFileStat: returns stat struct of a file via storage driver + * + * @src: file structure pointing to the file + * @stat: stat structure to return data + * + * Returns 0 on success, -2 if the function isn't supported by the backend, + * -1 on other failure. Errno is set in case of failure. +*/ +int +virStorageFileStat(virStorageSourcePtr src, + struct stat *st) +{ + int ret; + + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileStat) { + errno = ENOSYS; + return -2; + } + + ret = src->drv->backend->storageFileStat(src, st); + + VIR_DEBUG("stat of storage file %p: ret=%d, errno=%d", + src, ret, errno); + + return ret; +} + + +/** + * virStorageFileReadHeader: read the beginning bytes of a file into a buffer + * + * @src: file structure pointing to the file + * @max_len: maximum number of bytes read from the storage file + * @buf: buffer to read the data into. buffer shall be freed by caller) + * + * Returns the count of bytes read on success and -1 on failure, -2 if the + * function isn't supported by the backend. + * Libvirt error is reported on failure. + */ +ssize_t +virStorageFileReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf) +{ + ssize_t ret; + + if (!virStorageFileIsInitialized(src)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("storage file backend not initialized")); + return -1; + } + + if (!src->drv->backend->storageFileReadHeader) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("storage file header reading is not supported for " + "storage type %s (protocol: %s)"), + virStorageTypeToString(src->type), + virStorageNetProtocolTypeToString(src->protocol)); + return -2; + } + + ret = src->drv->backend->storageFileReadHeader(src, max_len, buf); + + VIR_DEBUG("read of storage header %p: ret=%zd", src, ret); + + return ret; +} + + +/* + * virStorageFileGetUniqueIdentifier: Get a unique string describing the volume + * + * @src: file structure pointing to the file + * + * Returns a string uniquely describing a single volume (canonical path). + * The string shall not be freed and is valid until the storage file is + * deinitialized. Returns NULL on error and sets a libvirt error code */ +const char * +virStorageFileGetUniqueIdentifier(virStorageSourcePtr src) +{ + if (!virStorageFileIsInitialized(src)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("storage file backend not initialized")); + return NULL; + } + + if (!src->drv->backend->storageFileGetUniqueIdentifier) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unique storage file identifier not implemented for " + "storage type %s (protocol: %s)'"), + virStorageTypeToString(src->type), + virStorageNetProtocolTypeToString(src->protocol)); + return NULL; + } + + return src->drv->backend->storageFileGetUniqueIdentifier(src); +} + + +/** + * virStorageFileAccess: Check accessibility of a storage file + * + * @src: storage file to check access permissions + * @mode: accessibility check options (see man 2 access) + * + * Returns 0 on success, -1 on error and sets errno. No libvirt + * error is reported. Returns -2 if the operation isn't supported + * by libvirt storage backend. + */ +int +virStorageFileAccess(virStorageSourcePtr src, + int mode) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileAccess) { + errno = ENOSYS; + return -2; + } + + return src->drv->backend->storageFileAccess(src, mode); +} + + +/** + * virStorageFileChown: Change owner of a storage file + * + * @src: storage file to change owner of + * @uid: new owner id + * @gid: new group id + * + * Returns 0 on success, -1 on error and sets errno. No libvirt + * error is reported. Returns -2 if the operation isn't supported + * by libvirt storage backend. + */ +int +virStorageFileChown(const virStorageSource *src, + uid_t uid, + gid_t gid) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileChown) { + errno = ENOSYS; + return -2; + } + + VIR_DEBUG("chown of storage file %p to %u:%u", + src, (unsigned int)uid, (unsigned int)gid); + + return src->drv->backend->storageFileChown(src, uid, gid); +} + + +/* Recursive workhorse for virStorageFileGetMetadata. */ +static int +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + virStorageSourcePtr parent, + uid_t uid, gid_t gid, + bool allow_probe, + bool report_broken, + virHashTablePtr cycle) +{ + int ret = -1; + const char *uniqueName; + char *buf = NULL; + ssize_t headerLen; + virStorageSourcePtr backingStore = NULL; + int backingFormat; + + 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; + + if (virStorageFileInitAs(src, uid, gid) < 0) + return -1; + + if (virStorageFileAccess(src, F_OK) < 0) { + if (src == parent) { + virReportSystemError(errno, + _("Cannot access storage file '%s' " + "(as uid:%u, gid:%u)"), + src->path, (unsigned int)uid, + (unsigned int)gid); + } else { + virReportSystemError(errno, + _("Cannot access backing file '%s' " + "of storage file '%s' (as uid:%u, gid:%u)"), + src->path, parent->path, + (unsigned int)uid, (unsigned int)gid); + } + + goto cleanup; + } + + if (!(uniqueName = virStorageFileGetUniqueIdentifier(src))) + goto cleanup; + + if (virHashLookup(cycle, uniqueName)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s (%s) is self-referential"), + src->path, uniqueName); + goto cleanup; + } + + if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0) + goto cleanup; + + if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) + goto cleanup; + + if (virStorageFileGetMetadataInternal(src, buf, headerLen, + &backingFormat) < 0) + goto cleanup; + + /* check whether we need to go deeper */ + if (!src->backingStoreRaw) { + ret = 0; + goto cleanup; + } + + if (!(backingStore = virStorageSourceNewFromBacking(src))) + goto cleanup; + + if (backingFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + backingStore->format = VIR_STORAGE_FILE_RAW; + else if (backingFormat == VIR_STORAGE_FILE_AUTO_SAFE) + backingStore->format = VIR_STORAGE_FILE_AUTO; + else + backingStore->format = backingFormat; + + if ((ret = virStorageFileGetMetadataRecurse(backingStore, parent, + uid, gid, + allow_probe, report_broken, + cycle)) < 0) { + if (report_broken) + goto cleanup; + + /* if we fail somewhere midway, just accept and return a + * broken chain */ + ret = 0; + goto cleanup; + } + + src->backingStore = backingStore; + backingStore = NULL; + ret = 0; + + cleanup: + VIR_FREE(buf); + virStorageFileDeinit(src); + virStorageSourceFree(backingStore); + return ret; +} + + +/** + * virStorageFileGetMetadata: + * + * Extract metadata about the storage volume with the specified + * image format. If image format is VIR_STORAGE_FILE_AUTO, it + * will probe to automatically identify the format. Recurses through + * the entire chain. + * + * Open files using UID and GID (or pass -1 for the current user/group). + * Treat any backing files without explicit type as raw, unless ALLOW_PROBE. + * + * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a + * format, since a malicious guest can turn a raw file into any + * other non-raw format at will. + * + * If @report_broken is true, the whole function fails with a possibly sane + * error instead of just returning a broken chain. + * + * Caller MUST free result after use via virStorageSourceFree. + */ +int +virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe, + bool report_broken) +{ + VIR_DEBUG("path=%s format=%d uid=%u gid=%u probe=%d, report_broken=%d", + src->path, src->format, (unsigned int)uid, (unsigned int)gid, + allow_probe, report_broken); + + virHashTablePtr cycle = NULL; + int ret = -1; + + if (!(cycle = virHashCreate(5, NULL))) + return -1; + + if (src->format <= VIR_STORAGE_FILE_NONE) + src->format = allow_probe ? + VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; + + ret = virStorageFileGetMetadataRecurse(src, src, uid, gid, + allow_probe, report_broken, cycle); + + virHashFree(cycle); + return ret; +} + + +/** + * virStorageFileGetBackingStoreStr: + * @src: storage object + * + * Extracts the backing store string as stored in the storage volume described + * by @src and returns it to the user. Caller is responsible for freeing it. + * In case when the string can't be retrieved or does not exist NULL is + * returned. + */ +char * +virStorageFileGetBackingStoreStr(virStorageSourcePtr src) +{ + virStorageSourcePtr tmp = NULL; + char *buf = NULL; + ssize_t headerLen; + char *ret = NULL; + + /* exit if we can't load information about the current image */ + if (!virStorageFileSupportsBackingChainTraversal(src)) + return NULL; + + if (virStorageFileAccess(src, F_OK) < 0) + return NULL; + + if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) + return NULL; + + if (!(tmp = virStorageSourceCopy(src, false))) + goto cleanup; + + if (virStorageFileGetMetadataInternal(tmp, buf, headerLen, NULL) < 0) + goto cleanup; + + VIR_STEAL_PTR(ret, tmp->backingStoreRaw); + + cleanup: + VIR_FREE(buf); + virStorageSourceFree(tmp); + + return ret; +} diff --git a/src/storage/storage_source.h b/src/storage/storage_source.h new file mode 100644 index 000000000..6b3362244 --- /dev/null +++ b/src/storage/storage_source.h @@ -0,0 +1,53 @@ +/* + * storage_source.h: Storage source accessors to real storaget + * + * 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_SOURCE_H__ +# define __VIR_STORAGE_SOURCE_H__ + +# include <sys/stat.h> + +# include "virstoragefile.h" + +int virStorageFileInit(virStorageSourcePtr src); +int virStorageFileInitAs(virStorageSourcePtr src, + uid_t uid, gid_t gid); +void virStorageFileDeinit(virStorageSourcePtr src); + +int virStorageFileCreate(virStorageSourcePtr src); +int virStorageFileUnlink(virStorageSourcePtr src); +int virStorageFileStat(virStorageSourcePtr src, + struct stat *stat); +ssize_t virStorageFileReadHeader(virStorageSourcePtr src, + ssize_t max_len, + char **buf); +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); + +int virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe, + bool report_broken) + ATTRIBUTE_NONNULL(1); + +char *virStorageFileGetBackingStoreStr(virStorageSourcePtr src) + ATTRIBUTE_NONNULL(1); + +#endif /* __VIR_STORAGE_SOURCE_H__ */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index a73c53839..d630a213f 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -32,6 +32,7 @@ #include "dirname.h" #include "storage/storage_driver.h" +#include "storage/storage_source.h" #define VIR_FROM_THIS VIR_FROM_NONE -- 2.12.2
On 06/23/2017 09:33 AM, Peter Krempa wrote:
The helper methods for actually accessing the storage objects don't really belong to the main storage driver implementation file. Split them out. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 1 + src/security/virt-aa-helper.c | 2 +- src/storage/storage_driver.c | 551 +-------------------------------------- src/storage/storage_driver.h | 28 -- src/storage/storage_source.c | 585 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_source.h | 53 ++++ tests/virstoragetest.c | 1 + 10 files changed, 645 insertions(+), 579 deletions(-) create mode 100644 src/storage/storage_source.c create mode 100644 src/storage/storage_source.h
Since all of the helpers being moved are prefixed with virStorageFile why not "storage_file.{c,h}"? I realize the helpers are all operating on virStorageSourcePtr. Is it perhaps because being too close to virstoragefile.{c,h}? It's not that important, but I suppose I'd expect virStorageSource helper prefixes in a storage_source.c. If you do decide to change the name, be sure to adjust the VIR_LOG_INIT appropriately too. And you could have also adjusted the .h prototypes to follow the more recently decided upon format that follows the .c files. As an aside - the remaining functions at the bottom of storage_driver probably could move to storage_util. Yeah, I know - patches are welcome. I'd prefer to see storage_file, but again it's not that important... Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Sat, Jul 01, 2017 at 10:42:56 -0400, John Ferlan wrote:
On 06/23/2017 09:33 AM, Peter Krempa wrote:
The helper methods for actually accessing the storage objects don't really belong to the main storage driver implementation file. Split them out. --- po/POTFILES.in | 1 + src/Makefile.am | 1 + src/qemu/qemu_domain.c | 1 + src/qemu/qemu_driver.c | 1 + src/security/virt-aa-helper.c | 2 +- src/storage/storage_driver.c | 551 +-------------------------------------- src/storage/storage_driver.h | 28 -- src/storage/storage_source.c | 585 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_source.h | 53 ++++ tests/virstoragetest.c | 1 + 10 files changed, 645 insertions(+), 579 deletions(-) create mode 100644 src/storage/storage_source.c create mode 100644 src/storage/storage_source.h
Since all of the helpers being moved are prefixed with virStorageFile why not "storage_file.{c,h}"? I realize the helpers are all operating on virStorageSourcePtr. Is it perhaps because being too close to virstoragefile.{c,h}?
The thing is that the virStorageSource does not necessarily need to be a file so I'd eventually like to rename those helpers, so I'll stick with this filename for now.
Allow specifying offset to read an arbitrary position in the file. This warrants a rename to virStorageFileRead. --- src/qemu/qemu_driver.c | 3 +-- src/storage/storage_backend.h | 9 +++++---- src/storage/storage_backend_fs.c | 20 +++++++++++++------ src/storage/storage_backend_gluster.c | 37 ++++++++++++++++++++++------------- src/storage/storage_source.c | 30 +++++++++++++++------------- src/storage/storage_source.h | 7 ++++--- 6 files changed, 63 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 052c7f0fc..4c4a6a036 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11584,8 +11584,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, goto cleanup; } } else { - if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) + if ((len = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, &buf)) < 0) goto cleanup; } diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 21ac84552..193cf134d 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -154,9 +154,10 @@ typedef int struct stat *st); typedef ssize_t -(*virStorageFileBackendReadHeader)(virStorageSourcePtr src, - ssize_t max_len, - char **buf); +(*virStorageFileBackendRead)(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf); typedef const char * (*virStorageFileBackendGetUniqueIdentifier)(virStorageSourcePtr src); @@ -186,7 +187,7 @@ struct _virStorageFileBackend { * error on failure. */ virStorageFileBackendInit backendInit; virStorageFileBackendDeinit backendDeinit; - virStorageFileBackendReadHeader storageFileReadHeader; + virStorageFileBackendRead storageFileRead; virStorageFileBackendGetUniqueIdentifier storageFileGetUniqueIdentifier; /* The following group of callbacks is expected to set errno diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index bf1d7de43..847dbc9e0 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -779,9 +779,10 @@ virStorageFileBackendFileStat(virStorageSourcePtr src, static ssize_t -virStorageFileBackendFileReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf) +virStorageFileBackendFileRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) { int fd = -1; ssize_t ret = -1; @@ -793,7 +794,14 @@ virStorageFileBackendFileReadHeader(virStorageSourcePtr src, return -1; } - if ((ret = virFileReadHeaderFD(fd, max_len, buf)) < 0) { + 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; @@ -850,7 +858,7 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileCreate = virStorageFileBackendFileCreate, .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, - .storageFileReadHeader = virStorageFileBackendFileReadHeader, + .storageFileRead = virStorageFileBackendFileRead, .storageFileAccess = virStorageFileBackendFileAccess, .storageFileChown = virStorageFileBackendFileChown, @@ -865,7 +873,7 @@ virStorageFileBackend virStorageFileBackendBlock = { .backendDeinit = virStorageFileBackendFileDeinit, .storageFileStat = virStorageFileBackendFileStat, - .storageFileReadHeader = virStorageFileBackendFileReadHeader, + .storageFileRead = virStorageFileBackendFileRead, .storageFileAccess = virStorageFileBackendFileAccess, .storageFileChown = virStorageFileBackendFileChown, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 93dce4042..8ea7e603c 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -151,20 +151,20 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) static ssize_t -virStorageBackendGlusterReadHeader(glfs_fd_t *fd, - const char *name, - ssize_t maxlen, - char **buf) +virStorageBackendGlusterRead(glfs_fd_t *fd, + const char *name, + size_t len, + char **buf) { char *s; size_t nread = 0; - if (VIR_ALLOC_N(*buf, maxlen) < 0) + if (VIR_ALLOC_N(*buf, len) < 0) return -1; s = *buf; - while (maxlen) { - ssize_t r = glfs_read(fd, s, maxlen, 0); + while (len) { + ssize_t r = glfs_read(fd, s, len, 0); if (r < 0 && errno == EINTR) continue; if (r < 0) { @@ -175,7 +175,7 @@ virStorageBackendGlusterReadHeader(glfs_fd_t *fd, if (r == 0) return nread; s += r; - maxlen -= r; + len -= r; nread += r; } return nread; @@ -292,7 +292,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; } - if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0) + if ((len = virStorageBackendGlusterRead(fd, name, len, &header)) < 0) goto cleanup; if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, @@ -721,9 +721,10 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src, static ssize_t -virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf) +virStorageFileBackendGlusterRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; glfs_fd_t *fd = NULL; @@ -737,8 +738,16 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, return -1; } - ret = virStorageBackendGlusterReadHeader(fd, src->path, max_len, buf); + 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); @@ -851,7 +860,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileCreate = virStorageFileBackendGlusterCreate, .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, - .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, + .storageFileRead = virStorageFileBackendGlusterRead, .storageFileAccess = virStorageFileBackendGlusterAccess, .storageFileChown = virStorageFileBackendGlusterChown, diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c index c91d629c8..130c2d2f3 100644 --- a/src/storage/storage_source.c +++ b/src/storage/storage_source.c @@ -64,7 +64,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) } return backend->storageFileGetUniqueIdentifier && - backend->storageFileReadHeader && + backend->storageFileRead && backend->storageFileAccess; } @@ -263,10 +263,11 @@ virStorageFileStat(virStorageSourcePtr src, /** - * virStorageFileReadHeader: read the beginning bytes of a file into a buffer + * virStorageFileRead: read the beginning bytes of a file into a buffer * * @src: file structure pointing to the file - * @max_len: maximum number of bytes read from the storage file + * @offset: number of bytes to skip in the storage file + * @len: maximum number of bytes read from the storage file * @buf: buffer to read the data into. buffer shall be freed by caller) * * Returns the count of bytes read on success and -1 on failure, -2 if the @@ -274,9 +275,10 @@ virStorageFileStat(virStorageSourcePtr src, * Libvirt error is reported on failure. */ ssize_t -virStorageFileReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf) +virStorageFileRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) { ssize_t ret; @@ -286,18 +288,18 @@ virStorageFileReadHeader(virStorageSourcePtr src, return -1; } - if (!src->drv->backend->storageFileReadHeader) { + if (!src->drv->backend->storageFileRead) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("storage file header reading is not supported for " + _("storage file reading is not supported for " "storage type %s (protocol: %s)"), virStorageTypeToString(src->type), virStorageNetProtocolTypeToString(src->protocol)); return -2; } - ret = src->drv->backend->storageFileReadHeader(src, max_len, buf); + ret = src->drv->backend->storageFileRead(src, offset, len, buf); - VIR_DEBUG("read of storage header %p: ret=%zd", src, ret); + VIR_DEBUG("read of storage %p: ret=%zd", src, ret); return ret; } @@ -444,8 +446,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0) goto cleanup; - if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) + if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) goto cleanup; if (virStorageFileGetMetadataInternal(src, buf, headerLen, @@ -565,8 +567,8 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src) if (virStorageFileAccess(src, F_OK) < 0) return NULL; - if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, - &buf)) < 0) + if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, + &buf)) < 0) return NULL; if (!(tmp = virStorageSourceCopy(src, false))) diff --git a/src/storage/storage_source.h b/src/storage/storage_source.h index 6b3362244..6462baf6a 100644 --- a/src/storage/storage_source.h +++ b/src/storage/storage_source.h @@ -32,9 +32,10 @@ int virStorageFileCreate(virStorageSourcePtr src); int virStorageFileUnlink(virStorageSourcePtr src); int virStorageFileStat(virStorageSourcePtr src, struct stat *stat); -ssize_t virStorageFileReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf); +ssize_t virStorageFileRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf); const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid); -- 2.12.2
On 06/23/2017 09:33 AM, Peter Krempa wrote:
Allow specifying offset to read an arbitrary position in the file. This warrants a rename to virStorageFileRead. --- src/qemu/qemu_driver.c | 3 +-- src/storage/storage_backend.h | 9 +++++---- src/storage/storage_backend_fs.c | 20 +++++++++++++------ src/storage/storage_backend_gluster.c | 37 ++++++++++++++++++++++------------- src/storage/storage_source.c | 30 +++++++++++++++------------- src/storage/storage_source.h | 7 ++++--- 6 files changed, 63 insertions(+), 43 deletions(-)
[...]
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 93dce4042..8ea7e603c 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -151,20 +151,20 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
static ssize_t -virStorageBackendGlusterReadHeader(glfs_fd_t *fd, - const char *name, - ssize_t maxlen, - char **buf) +virStorageBackendGlusterRead(glfs_fd_t *fd, + const char *name, + size_t len, + char **buf) { char *s; size_t nread = 0;
- if (VIR_ALLOC_N(*buf, maxlen) < 0) + if (VIR_ALLOC_N(*buf, len) < 0) return -1;
s = *buf; - while (maxlen) { - ssize_t r = glfs_read(fd, s, maxlen, 0); + while (len) { + ssize_t r = glfs_read(fd, s, len, 0); if (r < 0 && errno == EINTR) continue; if (r < 0) { @@ -175,7 +175,7 @@ virStorageBackendGlusterReadHeader(glfs_fd_t *fd, if (r == 0) return nread; s += r; - maxlen -= r; + len -= r; nread += r; } return nread; @@ -292,7 +292,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; }
Should : ssize_t len = VIR_STORAGE_MAX_HEADER; change to size_t and a new variable "ssize_t read_len" be created and used for the following and subsequent virStorageFileGetMetadataFromBuf call? (although that also takes a size_t for the 3rd param).
- if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0) + if ((len = virStorageBackendGlusterRead(fd, name, len, &header)) < 0) goto cleanup;
if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len, @@ -721,9 +721,10 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src,
static ssize_t -virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf) +virStorageFileBackendGlusterRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; glfs_fd_t *fd = NULL; @@ -737,8 +738,16 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src, return -1; }
- ret = virStorageBackendGlusterReadHeader(fd, src->path, max_len, buf); + 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);
@@ -851,7 +860,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileCreate = virStorageFileBackendGlusterCreate, .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, - .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, + .storageFileRead = virStorageFileBackendGlusterRead, .storageFileAccess = virStorageFileBackendGlusterAccess, .storageFileChown = virStorageFileBackendGlusterChown,
diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c index c91d629c8..130c2d2f3 100644 --- a/src/storage/storage_source.c +++ b/src/storage/storage_source.c @@ -64,7 +64,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) }
return backend->storageFileGetUniqueIdentifier && - backend->storageFileReadHeader && + backend->storageFileRead && backend->storageFileAccess; }
@@ -263,10 +263,11 @@ virStorageFileStat(virStorageSourcePtr src,
/** - * virStorageFileReadHeader: read the beginning bytes of a file into a buffer + * virStorageFileRead: read the beginning bytes of a file into a buffer
Probably should put the description below the args and restate since it reading @len bytes of data starting at @offset
* * @src: file structure pointing to the file - * @max_len: maximum number of bytes read from the storage file + * @offset: number of bytes to skip in the storage file + * @len: maximum number of bytes read from the storage file * @buf: buffer to read the data into. buffer shall be freed by caller) * * Returns the count of bytes read on success and -1 on failure, -2 if the @@ -274,9 +275,10 @@ virStorageFileStat(virStorageSourcePtr src, * Libvirt error is reported on failure. */ ssize_t -virStorageFileReadHeader(virStorageSourcePtr src, - ssize_t max_len, - char **buf) +virStorageFileRead(virStorageSourcePtr src, + size_t offset, + size_t len, + char **buf) { ssize_t ret;
@@ -286,18 +288,18 @@ virStorageFileReadHeader(virStorageSourcePtr src, return -1; }
- if (!src->drv->backend->storageFileReadHeader) { + if (!src->drv->backend->storageFileRead) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("storage file header reading is not supported for " + _("storage file reading is not supported for " "storage type %s (protocol: %s)"), virStorageTypeToString(src->type), virStorageNetProtocolTypeToString(src->protocol)); return -2; }
- ret = src->drv->backend->storageFileReadHeader(src, max_len, buf); + ret = src->drv->backend->storageFileRead(src, offset, len, buf);
- VIR_DEBUG("read of storage header %p: ret=%zd", src, ret); + VIR_DEBUG("read of storage %p: ret=%zd", src, ret);
"read %zd bytes from storage %p starting at offset %zu", ret, src, offset With a few tweaks: Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On Sat, Jul 01, 2017 at 10:43:09 -0400, John Ferlan wrote:
On 06/23/2017 09:33 AM, Peter Krempa wrote:
Allow specifying offset to read an arbitrary position in the file. This warrants a rename to virStorageFileRead. --- src/qemu/qemu_driver.c | 3 +-- src/storage/storage_backend.h | 9 +++++---- src/storage/storage_backend_fs.c | 20 +++++++++++++------ src/storage/storage_backend_gluster.c | 37 ++++++++++++++++++++++------------- src/storage/storage_source.c | 30 +++++++++++++++------------- src/storage/storage_source.h | 7 ++++--- 6 files changed, 63 insertions(+), 43 deletions(-)
[...]
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 93dce4042..8ea7e603c 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c
[...]
@@ -292,7 +292,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; }
Should :
ssize_t len = VIR_STORAGE_MAX_HEADER;
change to size_t and a new variable "ssize_t read_len" be created and used for the following and subsequent virStorageFileGetMetadataFromBuf call? (although that also takes a size_t for the 3rd param).
I actually just used VIR_STORAGE_MAX_HEADER constant directly in the call of virStorageBackendGlusterRead. The original variable will thus be used only for the return value, which is signed.
- if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0) + if ((len = virStorageBackendGlusterRead(fd, name, len, &header)) < 0) goto cleanup;
if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len,
Refactor the access to storage driver usage along with qemuDomainStorageFileInit which ensures that we access the file with correct DAC uid/gid. --- src/qemu/qemu_driver.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4c4a6a036..115368d1d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11309,9 +11309,9 @@ qemuDomainBlockPeek(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; + virDomainDiskDefPtr disk = NULL; virDomainObjPtr vm; - int fd = -1, ret = -1; - const char *actual; + int ret = -1; virCheckFlags(0, -1); @@ -11322,32 +11322,23 @@ qemuDomainBlockPeek(virDomainPtr dom, goto cleanup; /* Check the path belongs to this domain. */ - if (!(actual = virDomainDiskPathByName(vm->def, path))) { + if (!(disk = virDomainDiskByName(vm->def, path, true))) { virReportError(VIR_ERR_INVALID_ARG, - _("invalid path '%s'"), path); + _("invalid disk or path '%s'"), path); goto cleanup; } - path = actual; - fd = qemuOpenFile(driver, vm, path, O_RDONLY, NULL, NULL); - if (fd < 0) + if (qemuDomainStorageFileInit(driver, vm, disk->src) < 0) goto cleanup; - /* Seek and read. */ - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should - * be 64 bits on all platforms. - */ - if (lseek(fd, offset, SEEK_SET) == (off_t) -1 || - saferead(fd, buffer, size) == (ssize_t) -1) { - virReportSystemError(errno, - _("%s: failed to seek or read"), path); + if (virStorageFileRead(disk->src, offset, size, buffer) < 0) goto cleanup; - } ret = 0; cleanup: - VIR_FORCE_CLOSE(fd); + if (disk) + virStorageFileDeinit(disk->src); virDomainObjEndAPI(&vm); return ret; } -- 2.12.2
On 06/23/2017 09:33 AM, Peter Krempa wrote:
Refactor the access to storage driver usage along with qemuDomainStorageFileInit which ensures that we access the file with correct DAC uid/gid. --- src/qemu/qemu_driver.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
Irony is the comment at the end of qemuOpenFile: * This function should not be used on storage sources. Use * qemuDomainStorageFileInit and storage driver APIs if possible. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Sat, Jul 01, 2017 at 10:43:23 -0400, John Ferlan wrote:
On 06/23/2017 09:33 AM, Peter Krempa wrote:
Refactor the access to storage driver usage along with qemuDomainStorageFileInit which ensures that we access the file with correct DAC uid/gid. --- src/qemu/qemu_driver.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-)
Irony is the comment at the end of qemuOpenFile:
* This function should not be used on storage sources. Use * qemuDomainStorageFileInit and storage driver APIs if possible.
This message was added by: commit f7105d0e4a485bf7d9e878fd17e675d7f9d29f9f Author: Peter Krempa <pkrempa@redhat.com> Date: Wed May 10 12:28:38 2017 +0200 the usage of qemuOpenFile (prior to that, open() was used directly) was added in: commit b4a40dd92dc7e6f110b13f2353cb5343d1147227 Author: Martin Kletzander <mkletzan@redhat.com> Date: Fri May 24 18:26:26 2013 +0200 I was just lazy and did not bother fixing all places right away and added the warning that it shouldn't be reused more.
The API documents that it peeks into the VM disk. We can't do that currently for non raw images so report an error. --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 115368d1d..53655749b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11328,6 +11328,12 @@ qemuDomainBlockPeek(virDomainPtr dom, goto cleanup; } + if (disk->src->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("peeking is supported only for RAW disks")); + goto cleanup; + } + if (qemuDomainStorageFileInit(driver, vm, disk->src) < 0) goto cleanup; -- 2.12.2
On 06/23/2017 09:33 AM, Peter Krempa wrote:
The API documents that it peeks into the VM disk. We can't do that currently for non raw images so report an error. --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 115368d1d..53655749b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11328,6 +11328,12 @@ qemuDomainBlockPeek(virDomainPtr dom, goto cleanup; }
+ if (disk->src->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("peeking is supported only for RAW disks")); + goto cleanup;
Just noting that cleanup will call virStorageFileDeinit. Although virStorageFileInitAs wouldn't be called since tis won't call qemuDomainStorageFileInit, but it shouldn't matter since virStorageFileDeinit does check if virStorageFileIsInitialized - just a concern that some how some "future" change to Deinit does something that assumes Init was run. I would hope not, but just pointing it out... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ } + if (qemuDomainStorageFileInit(driver, vm, disk->src) < 0) goto cleanup;
participants (2)
-
John Ferlan -
Peter Krempa