Re: [libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS

2018-07-26 Thread Cole Robinson 
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 
>> ---
>>  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 @@
>>219100
>>1
>>
>> -hvm
>> +hvm
> 
> 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.
> 
>>  
>>
>>
>> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS

2018-07-26 Thread Michal Privoznik 
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 
> ---
>  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.

> -
> +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 @@
>219100
>1
>
> -hvm
> +hvm

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.

>  
>
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS

2018-07-24 Thread Cole Robinson 
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 
---
 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))) {
+if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))
 goto cleanup;
-
+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 @@
   219100
   1
   
-hvm
+hvm
 
   
   
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);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list