[libvirt] [PATCH 4/6] qemu: Report better errors from broken backing chains

[John Ferlan]

On 09/11/2014 01:47 PM, Peter Krempa wrote:
> Request erroring out from the backing chain traveller and drop qemu's
> internal backing chain integrity tester.
> 
> The backin chain traveller reports errors by itself with possibly more
> detail than qemuDiskChainCheckBroken ever could.

s/backin/backing/

> 
> We also need to make sure that we reconnect to existing qemu instances
> even at the cost of losing the backing chain info (this really should be
> stored in the XML rather than reloaded from disk, but that needs some
> work).
> ---
>  src/qemu/qemu_domain.c  | 26 ++------------------------
>  src/qemu/qemu_process.c |  5 ++---
>  2 files changed, 4 insertions(+), 27 deletions(-)
> 

With the number of callers would it be advisable to allow
qemuDomainDetermineDiskChain() callers to determine whether they want
virStorageFileGetMetadata to return the first broken...  Of course that
too has a wonderful variable name "force"  (force what? ;-))

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 88c5d1b..72638cf 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2440,27 +2440,6 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
>      return -1;
>  }
> 
> -static int
> -qemuDiskChainCheckBroken(virDomainDiskDefPtr disk)
> -{
> -    char *brokenFile = NULL;
> -
> -    if (!virDomainDiskGetSource(disk))
> -        return 0;
> -
> -    if (virStorageFileChainGetBroken(disk->src, &brokenFile) < 0)
> -        return -1;
> -
> -    if (brokenFile) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       _("Backing file '%s' of image '%s' is missing."),
> -                       brokenFile, virDomainDiskGetSource(disk));
> -        VIR_FREE(brokenFile);
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> 
>  int
>  qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
> @@ -2490,8 +2469,7 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
>              virFileExists(path))
>              continue;
> 
> -        if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0 &&
> -            qemuDiskChainCheckBroken(disk) >= 0)
> +        if (qemuDomainDetermineDiskChain(driver, vm, disk, false) >= 0)
>              continue;
> 
>          if (disk->startupPolicy &&
> @@ -2660,7 +2638,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>      if (virStorageFileGetMetadata(disk->src,
>                                    uid, gid,
>                                    cfg->allowDiskFormatProbing,
> -                                  false) < 0)
> +                                  true) < 0)
>          ret = -1;
> 
>   cleanup:
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 07335a0..a807f50 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3342,9 +3342,8 @@ qemuProcessReconnect(void *opaque)
>              goto error;
> 
>          /* XXX we should be able to restore all data from XML in the future */
> -        if (qemuDomainDetermineDiskChain(driver, obj,
> -                                         obj->def->disks[i], true) < 0)
> -            goto error;
> +        ignore_value(qemuDomainDetermineDiskChain(driver, obj,
> +                                                  obj->def->disks[i], true));

This is one case where it seems beneficial to have the Determine code be
able to ignore broken chains when in recursing, but yet still report the
"first" pass through (eg, when src == parent from patch 4).

With this change, this path will ignore the first pass through the
*Recurse() as well as failure from virHashCreate in
virStorageFileGetMetadata

Figure I'll let you consider whether that's acceptable as I wasn't 100%
if one could "pass" the virStorageFileSupportsBackingChainTraversal
call, but fail on any of the other calls to the *Recurse() call causing
failure which wouldn't be the chains, but the parent, right?

ACK in general - just considering the above odd error path and of course
allowing the callers to dictate whether they want the errors or not.

John
> 
>          dev.type = VIR_DOMAIN_DEVICE_DISK;
>          dev.data.disk = obj->def->disks[i];
>