[libvirt] [PATCHv3 03/36] storage: Move virStorageFileGetMetadata to the storage driver

My future work will modify the metadata crawler function to use the storage driver file APIs to access the files instead of accessing them directly so that we will be able to request the metadata for remote files too. To avoid linking the storage driver to every helper file using the utils code, the backing chain traversal function needs to be moved to the storage driver source. Additionally the virt-aa-helper and virstoragetest programs need to be linked with the storage driver as a result of this change. --- Notes: Version 3: - fixed whitespace issue - ACKed by Eric cfg.mk | 2 +- src/Makefile.am | 2 + src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 + src/security/virt-aa-helper.c | 2 + src/storage/storage_driver.c | 233 ++++++++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 5 + src/util/virstoragefile.c | 233 +----------------------------------------- src/util/virstoragefile.h | 7 +- tests/Makefile.am | 8 +- tests/virstoragetest.c | 2 + 11 files changed, 259 insertions(+), 239 deletions(-) diff --git a/cfg.mk b/cfg.mk index 5ff2721..9e8fcec 100644 --- a/cfg.mk +++ b/cfg.mk @@ -774,7 +774,7 @@ sc_prohibit_cross_inclusion: access/ | conf/) safe="($$dir|conf|util)";; \ locking/) safe="($$dir|util|conf|rpc)";; \ cpu/| network/| node_device/| rpc/| security/| storage/) \ - safe="($$dir|util|conf)";; \ + safe="($$dir|util|conf|storage)";; \ xenapi/ | xenxs/ ) safe="($$dir|util|conf|xen)";; \ *) safe="($$dir|$(mid_dirs)|util)";; \ esac; \ diff --git a/src/Makefile.am b/src/Makefile.am index cfb7097..5028f0d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2565,8 +2565,10 @@ virt_aa_helper_LDFLAGS = \ $(PIE_LDFLAGS) \ $(NULL) virt_aa_helper_LDADD = \ + libvirt.la \ libvirt_conf.la \ libvirt_util.la \ + libvirt_driver_storage_impl.la \ ../gnulib/lib/libgnu.la if WITH_DTRACE_PROBES virt_aa_helper_LDADD += libvirt_probes.lo diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cb635cd..91f13a4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1863,9 +1863,9 @@ virStorageFileFeatureTypeToString; virStorageFileFormatTypeFromString; virStorageFileFormatTypeToString; virStorageFileGetLVMKey; -virStorageFileGetMetadata; virStorageFileGetMetadataFromBuf; virStorageFileGetMetadataFromFD; +virStorageFileGetMetadataFromFDInternal; virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 78cfdc6..502b7bd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -40,6 +40,8 @@ #include "virstoragefile.h" #include "virstring.h" +#include "storage/storage_driver.h" + #include <sys/time.h> #include <fcntl.h> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 32fc04a..bf540b4 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -55,6 +55,8 @@ #include "virrandom.h" #include "virstring.h" +#include "storage/storage_driver.h" + #define VIR_FROM_THIS VIR_FROM_SECURITY static char *progname; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index f66c259..59f6fa8 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -49,6 +49,7 @@ #include "configmake.h" #include "virstring.h" #include "viraccessapicheck.h" +#include "dirname.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -3041,3 +3042,235 @@ virStorageFileAccess(virStorageSourcePtr src, return src->drv->backend->storageFileAccess(src, mode); } + + +/** + * Given a starting point START (a directory containing the original + * file, if the original file was opened via a relative path; ignored + * if NAME is absolute), determine the location of the backing file + * NAME (possibly relative), and compute the relative DIRECTORY + * (optional) and CANONICAL (mandatory) location of the backing file. + * Return 0 on success, negative on error. + */ +static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) +virFindBackingFile(const char *start, const char *path, + char **directory, char **canonical) +{ + /* FIXME - when we eventually allow non-raw network devices, we + * must ensure that we handle backing files the same way as qemu. + * For a qcow2 top file of gluster://server/vol/img, qemu treats + * the relative backing file 'rel' as meaning + * 'gluster://server/vol/rel', while the backing file '/abs' is + * used as a local file. But we cannot canonicalize network + * devices via canonicalize_file_name(), because they are not part + * of the local file system. */ + char *combined = NULL; + int ret = -1; + + if (*path == '/') { + /* Safe to cast away const */ + combined = (char *)path; + } else if (virAsprintf(&combined, "%s/%s", start, path) < 0) { + goto cleanup; + } + + if (directory && !(*directory = mdir_name(combined))) { + virReportOOMError(); + goto cleanup; + } + + if (virFileAccessibleAs(combined, F_OK, geteuid(), getegid()) < 0) { + virReportSystemError(errno, + _("Cannot access backing file '%s'"), + combined); + goto cleanup; + } + + if (!(*canonical = canonicalize_file_name(combined))) { + virReportSystemError(errno, + _("Can't canonicalize path '%s'"), path); + goto cleanup; + } + + ret = 0; + + cleanup: + if (combined != path) + VIR_FREE(combined); + return ret; +} + + +/* Recursive workhorse for virStorageFileGetMetadata. */ +static int +virStorageFileGetMetadataRecurse(virStorageSourcePtr src, + const char *canonPath, + uid_t uid, gid_t gid, + bool allow_probe, + virHashTablePtr cycle) +{ + int fd; + int ret = -1; + virStorageSourcePtr backingStore = NULL; + int backingFormat; + + VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", + src->path, canonPath, NULLSTR(src->relDir), src->format, + (int)uid, (int)gid, allow_probe); + + if (virHashLookup(cycle, canonPath)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s is self-referential"), + src->path); + return -1; + } + + if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) + return -1; + + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("Failed to open file '%s'"), + src->path); + return -1; + } + + if (virStorageFileGetMetadataFromFDInternal(src, fd, + &backingFormat) < 0) { + VIR_FORCE_CLOSE(fd); + return -1; + } + + if (VIR_CLOSE(fd) < 0) + VIR_WARN("could not close file %s", src->path); + } else { + /* TODO: currently we call this only for local storage */ + return 0; + } + + /* check whether we need to go deeper */ + if (!src->backingStoreRaw) + return 0; + + if (VIR_ALLOC(backingStore) < 0) + return -1; + + if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) + goto cleanup; + + if (virStorageIsFile(src->backingStoreRaw)) { + backingStore->type = VIR_STORAGE_TYPE_FILE; + + if (virFindBackingFile(src->relDir, + src->backingStoreRaw, + &backingStore->relDir, + &backingStore->path) < 0) { + /* the backing file is (currently) unavailable, treat this + * file as standalone: + * backingStoreRaw is kept to mark broken image chains */ + VIR_WARN("Backing file '%s' of image '%s' is missing.", + src->backingStoreRaw, src->path); + ret = 0; + goto cleanup; + } + } else { + /* TODO: To satisfy the test case, copy the network URI as path. This + * will be removed later. */ + backingStore->type = VIR_STORAGE_TYPE_NETWORK; + + if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) + 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 (virStorageFileGetMetadataRecurse(backingStore, + backingStore->path, + uid, gid, allow_probe, + cycle) < 0) { + /* if we fail somewhere midway, just accept and return a + * broken chain */ + ret = 0; + goto cleanup; + } + + src->backingStore = backingStore; + backingStore = NULL; + ret = 0; + + cleanup: + 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. + * + * Caller MUST free result after use via virStorageSourceFree. + */ +int +virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe) +{ + VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", + src->path, src->format, (int)uid, (int)gid, allow_probe); + + virHashTablePtr cycle = NULL; + char *canonPath = NULL; + int ret = -1; + + if (!(cycle = virHashCreate(5, NULL))) + return -1; + + if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { + if (!(canonPath = canonicalize_file_name(src->path))) { + virReportSystemError(errno, _("unable to resolve '%s'"), + src->path); + goto cleanup; + } + + if (!src->relPath && + VIR_STRDUP(src->relPath, src->path) < 0) + goto cleanup; + + if (!src->relDir && + !(src->relDir = mdir_name(src->path))) { + virReportOOMError(); + goto cleanup; + } + } else { + /* TODO: currently unimplemented for non-local storage */ + ret = 0; + goto cleanup; + } + + if (src->format <= VIR_STORAGE_FILE_NONE) + src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; + + ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, + allow_probe, cycle); + + cleanup: + VIR_FREE(canonPath); + virHashFree(cycle); + return ret; +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 5452df2..bd9e9ed 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -44,6 +44,11 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); +int virStorageFileGetMetadata(virStorageSourcePtr src, + uid_t uid, gid_t gid, + bool allow_probe) + ATTRIBUTE_NONNULL(1); + int storageRegister(void); #endif /* __VIR_STORAGE_DRIVER_H__ */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c9b6187..4956808 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -28,7 +28,6 @@ #include <unistd.h> #include <fcntl.h> #include <stdlib.h> -#include "dirname.h" #include "viralloc.h" #include "virerror.h" #include "virlog.h" @@ -565,62 +564,6 @@ qedGetBackingStore(char **res, return BACKING_STORE_OK; } -/** - * Given a starting point START (a directory containing the original - * file, if the original file was opened via a relative path; ignored - * if NAME is absolute), determine the location of the backing file - * NAME (possibly relative), and compute the relative DIRECTORY - * (optional) and CANONICAL (mandatory) location of the backing file. - * Return 0 on success, negative on error. - */ -static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) -virFindBackingFile(const char *start, const char *path, - char **directory, char **canonical) -{ - /* FIXME - when we eventually allow non-raw network devices, we - * must ensure that we handle backing files the same way as qemu. - * For a qcow2 top file of gluster://server/vol/img, qemu treats - * the relative backing file 'rel' as meaning - * 'gluster://server/vol/rel', while the backing file '/abs' is - * used as a local file. But we cannot canonicalize network - * devices via canonicalize_file_name(), because they are not part - * of the local file system. */ - char *combined = NULL; - int ret = -1; - - if (*path == '/') { - /* Safe to cast away const */ - combined = (char *)path; - } else if (virAsprintf(&combined, "%s/%s", start, path) < 0) { - goto cleanup; - } - - if (directory && !(*directory = mdir_name(combined))) { - virReportOOMError(); - goto cleanup; - } - - if (virFileAccessibleAs(combined, F_OK, geteuid(), getegid()) < 0) { - virReportSystemError(errno, - _("Cannot access backing file '%s'"), - combined); - goto cleanup; - } - - if (!(*canonical = canonicalize_file_name(combined))) { - virReportSystemError(errno, - _("Can't canonicalize path '%s'"), path); - goto cleanup; - } - - ret = 0; - - cleanup: - if (combined != path) - VIR_FREE(combined); - return ret; -} - static bool virStorageFileMatchesMagic(int format, @@ -1012,7 +955,7 @@ virStorageFileGetMetadataFromBuf(const char *path, /* Internal version that also supports a containing directory name. */ -static int +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, int fd, int *backingFormat) @@ -1111,180 +1054,6 @@ virStorageFileGetMetadataFromFD(const char *path, } -/* Recursive workhorse for virStorageFileGetMetadata. */ -static int -virStorageFileGetMetadataRecurse(virStorageSourcePtr src, - const char *canonPath, - uid_t uid, gid_t gid, - bool allow_probe, - virHashTablePtr cycle) -{ - int fd; - int ret = -1; - virStorageSourcePtr backingStore = NULL; - int backingFormat; - - VIR_DEBUG("path=%s canonPath=%s dir=%s format=%d uid=%d gid=%d probe=%d", - src->path, canonPath, NULLSTR(src->relDir), src->format, - (int)uid, (int)gid, allow_probe); - - if (virHashLookup(cycle, canonPath)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("backing store for %s is self-referential"), - src->path); - return -1; - } - - if (virHashAddEntry(cycle, canonPath, (void *)1) < 0) - return -1; - - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if ((fd = virFileOpenAs(src->path, O_RDONLY, 0, uid, gid, 0)) < 0) { - virReportSystemError(-fd, _("Failed to open file '%s'"), - src->path); - return -1; - } - - if (virStorageFileGetMetadataFromFDInternal(src, fd, - &backingFormat) < 0) { - VIR_FORCE_CLOSE(fd); - return -1; - } - - if (VIR_CLOSE(fd) < 0) - VIR_WARN("could not close file %s", src->path); - } else { - /* TODO: currently we call this only for local storage */ - return 0; - } - - /* check whether we need to go deeper */ - if (!src->backingStoreRaw) - return 0; - - if (VIR_ALLOC(backingStore) < 0) - return -1; - - if (VIR_STRDUP(backingStore->relPath, src->backingStoreRaw) < 0) - goto cleanup; - - if (virStorageIsFile(src->backingStoreRaw)) { - backingStore->type = VIR_STORAGE_TYPE_FILE; - - if (virFindBackingFile(src->relDir, - src->backingStoreRaw, - &backingStore->relDir, - &backingStore->path) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - VIR_WARN("Backing file '%s' of image '%s' is missing.", - src->backingStoreRaw, src->path); - ret = 0; - goto cleanup; - } - } else { - /* TODO: To satisfy the test case, copy the network URI as path. This - * will be removed later. */ - backingStore->type = VIR_STORAGE_TYPE_NETWORK; - - if (VIR_STRDUP(backingStore->path, src->backingStoreRaw) < 0) - 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 (virStorageFileGetMetadataRecurse(backingStore, - backingStore->path, - uid, gid, allow_probe, - cycle) < 0) { - /* if we fail somewhere midway, just accept and return a - * broken chain */ - ret = 0; - goto cleanup; - } - - src->backingStore = backingStore; - backingStore = NULL; - ret = 0; - - cleanup: - 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. - * - * Caller MUST free result after use via virStorageSourceFree. - */ -int -virStorageFileGetMetadata(virStorageSourcePtr src, - uid_t uid, gid_t gid, - bool allow_probe) -{ - VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d", - src->path, src->format, (int)uid, (int)gid, allow_probe); - - virHashTablePtr cycle = NULL; - char *canonPath = NULL; - int ret = -1; - - if (!(cycle = virHashCreate(5, NULL))) - return -1; - - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK) { - if (!(canonPath = canonicalize_file_name(src->path))) { - virReportSystemError(errno, _("unable to resolve '%s'"), - src->path); - goto cleanup; - } - - if (!src->relPath && - VIR_STRDUP(src->relPath, src->path) < 0) - goto cleanup; - - if (!src->relDir && - !(src->relDir = mdir_name(src->path))) { - virReportOOMError(); - goto cleanup; - } - } else { - /* TODO: currently unimplemented for non-local storage */ - ret = 0; - goto cleanup; - } - - if (src->format <= VIR_STORAGE_FILE_NONE) - src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW; - - ret = virStorageFileGetMetadataRecurse(src, canonPath, uid, gid, - allow_probe, cycle); - - cleanup: - VIR_FREE(canonPath); - virHashFree(cycle); - return ret; -} - /** * virStorageFileChainCheckBroken * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 158806b..9947203 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -265,10 +265,9 @@ struct _virStorageSource { int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); -int virStorageFileGetMetadata(virStorageSourcePtr src, - uid_t uid, gid_t gid, - bool allow_probe) - ATTRIBUTE_NONNULL(1); +int virStorageFileGetMetadataFromFDInternal(virStorageSourcePtr meta, + int fd, + int *backingFormat); virStorageSourcePtr virStorageFileGetMetadataFromFD(const char *path, int fd, int format, diff --git a/tests/Makefile.am b/tests/Makefile.am index 5ef8940..f9f2b84 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -869,7 +869,13 @@ virstringtest_LDADD = $(LDADDS) virstoragetest_SOURCES = \ virstoragetest.c testutils.h testutils.c -virstoragetest_LDADD = $(LDADDS) +virstoragetest_LDADD = $(LDADDS) \ + ../src/libvirt.la \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + ../src/libvirt_driver_storage_impl.la \ + ../gnulib/lib/libgnu.la \ + $(NULL) viridentitytest_SOURCES = \ viridentitytest.c testutils.h testutils.c diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index d49098e..fb96c71 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -31,6 +31,8 @@ #include "virstring.h" #include "dirname.h" +#include "storage/storage_driver.h" + #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("tests.storagetest"); -- 1.9.3