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

[Guannan Ren]

On 07/31/2013 04:49 PM, Martin Kletzander wrote:
> On 07/30/2013 08:26 AM, 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.
>> ---
>>   src/qemu/qemu_domain.c  | 31 +++++++++++++------------------
>>   src/qemu/qemu_process.c |  6 ------
>>   2 files changed, 13 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index be77991..1e75adb 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -2043,19 +2043,11 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
>>               break;
>>   
>>           case VIR_DOMAIN_STARTUP_POLICY_MANDATORY:
>> -            virReportSystemError(errno,
>> -                                 _("cannot access file '%s'"),
>> -                                 disk->src);
>>               goto error;
>> -            break;
>>   
>>           case VIR_DOMAIN_STARTUP_POLICY_REQUISITE:
>> -            if (cold_boot) {
>> -                virReportSystemError(errno,
>> -                                     _("cannot access file '%s'"),
>> -                                     disk->src);
>> +            if (cold_boot)
>>                   goto error;
>> -            }
>>               break;
>>   
>>           case VIR_DOMAIN_STARTUP_POLICY_DEFAULT:
>> @@ -2064,6 +2056,7 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
>>               break;
>>       }
>>   
>> +    virResetLastError();
> Even though it makes perfect sense to have it here, I'd move it one
> function up, because it seems more readable to me.
>
>>       VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') "
>>                 "due to inaccessible source '%s'",
>>                 disk->dst, vm->def->name, uuid, disk->src);
>> @@ -2091,22 +2084,24 @@ 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 &&
>> +            qemuDiskChainCheckBroken(disk) >= 0)
>> +                continue;
>> +
>> +        if (disk->startupPolicy) {
>> +            if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
>> +                                                 cold_boot) >= 0)
> And rewrite this to one condition.
>

It's okay to rewrite it to one condition, but existing format is more 
readable.
The outer if clause is used to check whether disk have startupPolicy set.
The inter if clause is used to do the actual startupPolicy work. Maybe 
later, we can do more work
for disk with startupPolicy attribute in its xml definition.

Guannan