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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list