[libvirt] [PATCH v2 3/3] qemu: check presence of each disk and its backing file as well

[Martin Kletzander]

On 07/26/2013 02:37 PM, Guannan Ren wrote:
> For disk with startupPolicy support, such as cdrom and floppy
> when its chain is broken, the startup policy will apply,
> otherwise, report an error on chain issue.
> ---
>  src/qemu/qemu_domain.c  | 21 ++++++++++++---------
>  src/qemu/qemu_process.c |  6 ------
>  2 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index be77991..b607454 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2091,22 +2091,25 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
>      virDomainDiskDefPtr disk;
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  
> +    VIR_DEBUG("Checking for disk presence");
>      for (i = 0; i < vm->def->ndisks; i++) {
>          disk = vm->def->disks[i];
>  
> -        if (!disk->startupPolicy || !disk->src)
> +        if (!disk->src)
>              continue;
>  
> -        if (virFileAccessibleAs(disk->src, F_OK,
> -                                cfg->user,
> -                                cfg->group) >= 0) {
> -            /* disk accessible */
> -            continue;
> +        if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0)
> +            if (qemuDiskChainCheckBroken(disk) >= 0)
> +                continue;

Multiline if(), this should be:

        if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 &&
            qemuDiskChainCheckBroken(disk) >= 0)
            continue;

Other than that, this for is very badly readable and every time I'm
reviewing a patch touching this loop I'm having problems wrapping my
head around it.

> +
> +        if (disk->startupPolicy) {
> +            virResetLastError();

This properly resets possible unrelated error which might have been
reported in qemuDomainDetermineDiskChain() for example, but if you have
backing chain broken and you have startupPolicy='mandatory', the error
you'll get when starting a domain will be "cannot access file:
<filename>: No such file or directory", but the file <filename> will
exist, the error will just be overridden.  And we definitely don't want
that.

The virResetLastError() makes sense only in the previously discussed
MetadataRecurse.  That breaks the test suite, but the check for the
WARN-ing is wrong there, so it should be fixed instead.

This loop should be reworked to always properly output an error that is
related to the situation, no errors should be shadowed etc.  Simply what
we are trying to do elsewhere.

Martin