diff mbox series

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

Message ID 09f9dd272a34ce25dd6f18a25c46645df24740d8.1532467269.git.crobinso@redhat.com
State Accepted
Commit cd9d439a716071867ff6c3324117a8ad9342024d
Headers show
Series conf: Replace SKIP_OSTYPE with SKIP_VALIDATE | expand

Commit Message

Cole Robinson July 24, 2018, 9:23 p.m. UTC 
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@redhat.com>

---
 src/conf/domain_conf.c                     | 13 ++++++-------
 tests/qemuxml2argvdata/missing-machine.xml |  2 +-
 tests/qemuxml2argvtest.c                   |  3 +++
 3 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.17.1

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

Comments

Michal Prívozník July 26, 2018, 6:44 a.m. UTC | #1 
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@redhat.com>

> ---

>  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 @@

>    <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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson July 26, 2018, 1:23 p.m. UTC | #2 
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@redhat.com>

>> ---

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

Patch

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 @@ 
   <currentMemory unit='KiB'>219100</currentMemory>
   <vcpu placement='static' cpuset='1'>1</vcpu>
   <os>
-    <type arch='i686'>hvm</type>
+    <type arch='alpha'>hvm</type>
     <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);