[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 08/16] storage: get entire metadata chain in one call



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]