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

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(a)redhat.com&gt; > --- > 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