[libvirt] [PATCH v2 3/5] qemu_domain.c: moving maxCpu validation to qemuDomainDefValidate

[Daniel Henrique Barboza]

On 11/15/18 7:38 PM, John Ferlan wrote:
>
> On 11/14/18 2:52 PM, Daniel Henrique Barboza wrote:
>> Adding maxCpu validation in qemuDomainDefValidate allows the user to
>> spot over the board maxCpus counts at editing time, instead of
>> facing a runtime error when starting the domain. This check is also
>> arch independent.
>>
>> This leaves us with 2 calls to qemuProcessValidateCpuCount: one in
>> qemuProcessStartValidateXML and the new one at qemuDomainDefValidate.
>>
>> The call in qemuProcessStartValidateXML is redundant. Following
>> up in that code, there is a call to virDomainDefValidate, which
>> in turn will call config.domainValidateCallback. In this case, the
>> callback function is qemuDomainDefValidate. This means that, on startup
>> time, qemuProcessValidateCpuCount will be called twice.
>>
>> To avoid that, let's also remove the qemuProcessValidateCpuCount call
>> from qemuProcessStartValidateXML.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   src/qemu/qemu_domain.c  |  4 ++++
>>   src/qemu/qemu_process.c | 14 +-------------
>>   2 files changed, 5 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 37926850b2..3b86d28216 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -4084,6 +4084,10 @@ qemuDomainDefValidate(const virDomainDef *def,
>>           }
>>       }
>>   
>> +    if (qemuProcessValidateCpuCount(def, qemuCaps) < 0) {
>> +        goto cleanup;
>> +    }
>> +
> make check would have told you to remove the { ... } for the one line
> goto cleanup;
>
> I'll take care of it for you (as well as the merge conflict it creates
> in the subsequent patch).
>
>

I appreciate it. I'll make sure to execute 'make check' before sending
the patch next time.


Thanks,


Daniel



>
> John
>
>>       if (ARCH_IS_X86(def->os.arch) &&
>>           virDomainDefGetVcpusMax(def) > QEMU_MAX_VCPUS_WITHOUT_EIM) {
>>           if (!qemuDomainIsQ35(def)) {
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 4d134bd7be..2adbf375ee 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -5140,21 +5140,9 @@ qemuProcessStartValidateDisks(virDomainObjPtr vm,
>>   static int
>>   qemuProcessStartValidateXML(virQEMUDriverPtr driver,
>>                               virDomainObjPtr vm,
>> -                            virQEMUCapsPtr qemuCaps,
>>                               virCapsPtr caps,
>>                               unsigned int flags)
>>   {
>> -    /* The bits we validate here are XML configs that we previously
>> -     * accepted. We reject them at VM startup time rather than parse
>> -     * time so that pre-existing VMs aren't rejected and dropped from
>> -     * the VM list when libvirt is updated.
>> -     *
>> -     * If back compat isn't a concern, XML validation should probably
>> -     * be done at parse time.
>> -     */
>> -    if (qemuProcessValidateCpuCount(vm->def, qemuCaps) < 0)
>> -        return -1;
>> -
>>       /* checks below should not be executed when starting a qemu process for a
>>        * VM that was running before (migration, snapshots, save). It's more
>>        * important to start such VM than keep the configuration clean */
>> @@ -5204,7 +5192,7 @@ qemuProcessStartValidate(virQEMUDriverPtr driver,
>>   
>>       }
>>   
>> -    if (qemuProcessStartValidateXML(driver, vm, qemuCaps, caps, flags) < 0)
>> +    if (qemuProcessStartValidateXML(driver, vm, caps, flags) < 0)
>>           return -1;
>>   
>>       if (qemuProcessStartValidateGraphics(vm) < 0)
>>