On 07/26/2018 02:44 AM, Michal Privoznik wrote:
> On 07/24/2018 11:23 PM, Cole Robinson wrote:
>> We should still make an effort to fill in data, just not raise
>> an error if say an ostype/virttype combo disappeared from caps.
>>
>> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>
>> ---
>> src/conf/domain_conf.c | 13 ++++++-------
>> tests/qemuxml2argvdata/missing-machine.xml | 2 +-
>> tests/qemuxml2argvtest.c | 3 +++
>> 3 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index b7f6a22e20..78ee000857 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -19178,18 +19178,17 @@ virDomainDefParseCaps(virDomainDefPtr def,
>> goto cleanup;
>> }
>>
>> - if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
>> - if (!(capsdata = virCapabilitiesDomainDataLookup(caps,
>> - def->os.type, def->os.arch, def->virtType,
>> - NULL, NULL)))
>> + if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
>> + def->os.arch, def->virtType, NULL, NULL))) {
>
> This looks misaligned ;-)
>
>> + if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))
>> goto cleanup;
>
>
> So you're changing the flag here even though I believe it belongs to the
> next patch. It helps downstream maintainers, but in the end the code
> will look the same.
>
Good catch, mistake on my part. I'll fix before pushing. Thanks for the
review!
- Cole
>> -
>> + virResetLastError();
>> + } else {
>> if (!def->os.arch)
>> def->os.arch = capsdata->arch;
>> if ((!def->os.machine &&
>> - VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) {
>> + VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0))
>> goto cleanup;
>> - }
>> }
>>
>> ret = 0;
>> diff --git a/tests/qemuxml2argvdata/missing-machine.xml b/tests/qemuxml2argvdata/missing-machine.xml
>> index 4ce7b377a5..2900baec90 100644
>> --- a/tests/qemuxml2argvdata/missing-machine.xml
>> +++ b/tests/qemuxml2argvdata/missing-machine.xml
>> @@ -6,7 +6,7 @@
>> <currentMemory unit='KiB'>219100</currentMemory>
>> <vcpu placement='static' cpuset='1'>1</vcpu>
>> <os>
>> - <type arch='i686'>hvm</type>
>> + <type arch='alpha'>hvm</type>
>
> Firstly I was wondering why is this change needed, but then I read the
> comment in the next hunk and it makes sense. We need to have
> non-existent pair of arch and os type so that the error is triggered.
>
>> <boot dev='hd'/>
>> </os>
>> <clock offset='utc'/>
>> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> index 1a936faef1..03b6d92912 100644
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -2773,6 +2773,9 @@ mymain(void)
>> QEMU_CAPS_OBJECT_GPEX,
>> QEMU_CAPS_NEC_USB_XHCI);
>>
>> + /* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag
>> + * will avoid the error. Still, we expect qemu driver to complain about
>> + * missing machine error, and not crash */
>> DO_TEST_PARSE_FLAGS_ERROR("missing-machine",
>> VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS,
>> NONE);
>>
>
> Michal
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
