Re: [libvirt] [PATCH 08/11] conf: Use domainPostParseData(Alloc|Free) in virDomainDefValidate

On 9/20/18 1:44 PM, Marc Hartmayer wrote: > For the usage of the parameter @parseOpqaue within > virDomainDefValidate it must be ensured that the parameter > @parseOpaque is not NULL. But since there are code paths where > virDomainDefValidate is called with @parseOpaque == > NULL (e.g. the function call of virDomainDefParseNode with @parseOpqaue == > NULL leads to this). > > To prevent this, domainPostParseDataAlloc is called within > virDomainDefValidate to ensure that @parseOpaque is always set. The > usage of domainPostParseDataAlloc within virDomainDefValidate is > allowed since virDomainDefValidate is only called after > virDomainDefPostParse. > > Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; > --- > src/conf/domain_conf.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > Thus is similar to commit e168bc8a did for virDomainDefPostParse...

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index ae7f3ed95faf..4f1c569b5733 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -6288,6 +6288,9 @@ virDomainDefValidate(virDomainDefPtr def, > virDomainXMLOptionPtr xmlopt, > void *parseOpaque) > { > + int ret = -1; > + bool localParseOpaque = false; > + > struct virDomainDefPostParseDeviceIteratorData data = { > .caps = caps, > .xmlopt = xmlopt, > @@ -6299,6 +6302,17 @@ virDomainDefValidate(virDomainDefPtr def, > if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE) > return 0; > > + if (!data.parseOpaque && > + xmlopt->config.domainPostParseDataAlloc) { > + ret = xmlopt->config.domainPostParseDataAlloc(def, caps, parseFlags, > + xmlopt->config.priv, > + &data.parseOpaque); So ret = 1 if virQEMUCapsCacheLookup failed in qemuDomainPostParseDataAlloc; otherwise, ret = 0; Just looks strange below with the ret == 1 check.

> + > + if (virDomainDefPostParseCheckFailure(def, parseFlags, ret) < 0) This though has/uses specific @parseFlag options and sets def->postParseFailed when it may not be true if for some reason of course the post parse processing never got the parseOpaque value. Wouldn't it perhaps be problematic in the patch3 scenario where you'd be passing the priv->qemuCaps when @postParseFailed?

John > + goto cleanup; > + localParseOpaque = true; > + } > + > /* call the domain config callback */ > if (xmlopt->config.domainValidateCallback && > xmlopt->config.domainValidateCallback(def, caps, xmlopt->config.priv, data.parseOpaque) < 0) > @@ -6313,6 +6327,13 @@ virDomainDefValidate(virDomainDefPtr def, > if (virDomainDefValidateInternal(def) < 0) > return -1; > > + cleanup: > + if (localParseOpaque && xmlopt->config.domainPostParseDataFree) > + xmlopt->config.domainPostParseDataFree(data.parseOpaque); > + > + if (ret == 1) > + return -1; > + > return 0; > } > >