[libvirt] [PATCHv2 1/4] util: storage: Allow metadata crawler to report useful errors

[John Ferlan]

On 09/18/2014 05:54 AM, Peter Krempa wrote:
> Add a new parameter to virStorageFileGetMetadata that will break the
> backing chain detection process and report useful error message rather
> than having to use virStorageFileChainGetBroken.
> 
> This patch just introduces the option, usage will be provided
> separately.
> ---
>  src/qemu/qemu_domain.c        |  3 ++-
>  src/security/virt-aa-helper.c |  2 +-
>  src/storage/storage_driver.c  | 24 +++++++++++++++++-------
>  src/storage/storage_driver.h  |  3 ++-
>  tests/virstoragetest.c        |  2 +-
>  5 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 5859ba7..19b935d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2693,7 +2693,8 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> 
>      if (virStorageFileGetMetadata(disk->src,
>                                    uid, gid,
> -                                  cfg->allowDiskFormatProbing) < 0)
> +                                  cfg->allowDiskFormatProbing,
> +                                  false) < 0)
>          ret = -1;
> 
>   cleanup:
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index a06ba44..9afc8db 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -932,7 +932,7 @@ get_files(vahControl * ctl)
>           */
>          if (!disk->src->backingStore) {
>              bool probe = ctl->allowDiskFormatProbing;
> -            virStorageFileGetMetadata(disk->src, -1, -1, probe);
> +            virStorageFileGetMetadata(disk->src, -1, -1, probe, false);
>          }
> 
>          /* XXX passing ignoreOpenFailure = true to get back to the behavior
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 433d7b7..c3b29c4 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2783,6 +2783,7 @@ static int
>  virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>                                   uid_t uid, gid_t gid,
>                                   bool allow_probe,
> +                                 bool report_broken,
>                                   virHashTablePtr cycle)
>  {
>      int ret = -1;
> @@ -2847,9 +2848,13 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>      else
>          backingStore->format = backingFormat;
> 
> -    if (virStorageFileGetMetadataRecurse(backingStore,
> -                                         uid, gid, allow_probe,
> -                                         cycle) < 0) {
> +    if ((ret = virStorageFileGetMetadataRecurse(backingStore,
> +                                                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;
> @@ -2883,15 +2888,20 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
>   * 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 allow_probe,
> +                          bool report_broken)
>  {
> -    VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d",
> -              src->path, src->format, (int)uid, (int)gid, allow_probe);
> +    VIR_DEBUG("path=%s format=%d uid=%d gid=%d probe=%d, report_broken=%d",
> +              src->path, src->format, (int)uid, (int)gid,
> +              allow_probe, report_broken);
> 
>      virHashTablePtr cycle = NULL;
>      int ret = -1;
> @@ -2903,7 +2913,7 @@ virStorageFileGetMetadata(virStorageSourcePtr src,
>          src->format = allow_probe ? VIR_STORAGE_FILE_AUTO : VIR_STORAGE_FILE_RAW;
> 
>      ret = virStorageFileGetMetadataRecurse(src, uid, gid,
> -                                           allow_probe, cycle);
> +                                           allow_probe, report_broken, cycle);
> 
>      virHashFree(cycle);
>      return ret;
> diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h
> index e773928..54a17a3 100644
> --- a/src/storage/storage_driver.h
> +++ b/src/storage/storage_driver.h
> @@ -50,7 +50,8 @@ bool virStorageFileSupportsSecurityDriver(virStorageSourcePtr src);
> 
>  int virStorageFileGetMetadata(virStorageSourcePtr src,
>                                uid_t uid, gid_t gid,
> -                              bool allow_probe)
> +                              bool allow_probe,
> +                              bool fail)

s/fail/report_broken

ACK

John
>      ATTRIBUTE_NONNULL(1);
> 
>  int virStorageTranslateDiskSourcePool(virConnectPtr conn,
> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index e2ee3ff..29f5c7a 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -119,7 +119,7 @@ testStorageFileGetMetadata(const char *path,
>      if (VIR_STRDUP(ret->path, path) < 0)
>          goto error;
> 
> -    if (virStorageFileGetMetadata(ret, uid, gid, allow_probe) < 0)
> +    if (virStorageFileGetMetadata(ret, uid, gid, allow_probe, false) < 0)
>          goto error;
> 
>      return ret;
>