On 10/13/2012 06:00 PM, Eric Blake wrote:
Previously, no one was using virStorageFileGetMetadata, and for good reason - it couldn't support root-squash NFS. Change the signature and make it useful to future patches, including enhancing the metadata to recursively track the entire chain.
* src/util/storage_file.h (_virStorageFileMetadata): Add field. (virStorageFileGetMetadata): Alter signature. * src/util/storage_file.c (virStorageFileGetMetadata): Rewrite. (virStorageFileGetMetadataRecurse): New function. (virStorageFileFreeMetadata): Handle recursion. --- src/util/storage_file.c | 94 +++++++++++++++++++++++++++++++++++++++---------- src/util/storage_file.h | 18 ++++++---- 2 files changed, 86 insertions(+), 26 deletions(-)
diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 3d1b7cf..0f879bd 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -40,6 +40,7 @@ #include "logging.h" #include "virfile.h" #include "c-ctype.h" +#include "virhash.h"
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -840,7 +841,7 @@ virStorageFileProbeFormat(const char *path) * * 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. + * will probe to automatically identify the format. Does not recurse. * * Callers are advised never to use VIR_STORAGE_FILE_AUTO as a * format, since a malicious guest can turn a raw file into any @@ -910,12 +911,69 @@ cleanup: return ret; }
+/* Recursive workhorse for virStorageFileGetMetadata. */ +static virStorageFileMetadataPtr +virStorageFileGetMetadataRecurse(const char *path, int format, + uid_t uid, gid_t gid, + bool allow_probe, virHashTablePtr cycle) +{ + int fd; + virStorageFileMetadataPtr ret = NULL; + + if (virHashLookup(cycle, path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("backing store for %s is self-referential"), + path); + return NULL; + } + if (virHashAddEntry(cycle, path, (void *)1) < 0) + return NULL; + + if ((fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, 0)) < 0) { + virReportSystemError(-fd, _("cannot open file '%s'"), path); + return NULL; + } + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(); + VIR_FORCE_CLOSE(fd); + return NULL; + } + if (virStorageFileGetMetadataFromFD(path, fd, format, ret) < 0) { + virStorageFileFreeMetadata(ret); + VIR_FORCE_CLOSE(fd); + return NULL; + } + + if (VIR_CLOSE(fd) < 0) + virReportSystemError(errno, _("could not close file %s"), path);
If this isn't fatal to the operation, shouldn't it be a warning instead of an error? (And if it is fatal, it should free the remaining resources and return NULL)
+ + if (ret->backingStoreIsFile) { + if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO && !allow_probe) + ret->backingStoreFormat = VIR_STORAGE_FILE_RAW; + else if (ret->backingStoreFormat == VIR_STORAGE_FILE_AUTO_SAFE) + ret->backingStoreFormat = VIR_STORAGE_FILE_AUTO; + format = ret->backingStoreFormat;
Why are you bothering to set this, instead of just using ret->backingStoreFormat in the args to the following function call? (It's harmless, though, so no big deal)
+ ret->backingMeta = virStorageFileGetMetadataRecurse(ret->backingStore, + format, + uid, gid, + allow_probe, + cycle); + } + + 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. + * 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 @@ -923,27 +981,24 @@ cleanup: * * If the returned meta.backingStoreFormat is VIR_STORAGE_FILE_AUTO * it indicates the image didn't specify an explicit format for its - * backing store. Callers are advised against probing for the - * backing store format in this case. + * backing store. Callers are advised against using ALLOW_PROBE, as + * it would probe the backing store format in this case. * - * Caller MUST free @meta after use via virStorageFileFreeMetadata. + * Caller MUST free result after use via virStorageFileFreeMetadata. */ -int -virStorageFileGetMetadata(const char *path, - int format, - virStorageFileMetadata *meta) +virStorageFileMetadataPtr +virStorageFileGetMetadata(const char *path, int format, + uid_t uid, gid_t gid, + bool allow_probe) { - int fd, ret; - - if ((fd = open(path, O_RDONLY)) < 0) { - virReportSystemError(errno, _("cannot open file '%s'"), path); - return -1; - } - - ret = virStorageFileGetMetadataFromFD(path, fd, format, meta); - - VIR_FORCE_CLOSE(fd); + virHashTablePtr cycle = virHashCreate(5, NULL); + virStorageFileMetadataPtr ret;
+ if (!cycle) + return NULL; + ret = virStorageFileGetMetadataRecurse(path, format, uid, gid, + allow_probe, cycle); + virHashFree(cycle); return ret; }
@@ -958,6 +1013,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return;
+ virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta); } diff --git a/src/util/storage_file.h b/src/util/storage_file.h index 683ec73..18dea6a 100644 --- a/src/util/storage_file.h +++ b/src/util/storage_file.h @@ -50,13 +50,16 @@ enum virStorageFileFormat {
VIR_ENUM_DECL(virStorageFileFormat);
-typedef struct _virStorageFileMetadata { +typedef struct _virStorageFileMetadata virStorageFileMetadata; +typedef virStorageFileMetadata *virStorageFileMetadataPtr; +struct _virStorageFileMetadata { char *backingStore; - int backingStoreFormat; + int backingStoreFormat; /* enum virStorageFileFormat */ bool backingStoreIsFile; + virStorageFileMetadataPtr backingMeta; unsigned long long capacity; bool encrypted; -} virStorageFileMetadata; +};
# ifndef DEV_BSIZE # define DEV_BSIZE 512 @@ -66,15 +69,16 @@ int virStorageFileProbeFormat(const char *path); int virStorageFileProbeFormatFromFD(const char *path, int fd);
-int virStorageFileGetMetadata(const char *path, - int format, - virStorageFileMetadata *meta); +virStorageFileMetadataPtr virStorageFileGetMetadata(const char *path, + int format, + uid_t uid, gid_t gid, + bool allow_probe); int virStorageFileGetMetadataFromFD(const char *path, int fd, int format, virStorageFileMetadata *meta);
-void virStorageFileFreeMetadata(virStorageFileMetadata *meta); +void virStorageFileFreeMetadata(virStorageFileMetadataPtr meta);
int virStorageFileResize(const char *path, unsigned long long capacity);
ACK, assuming acceptable response to my above questions.