On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan <jferlan@redhat.com> wrote:
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@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/conf/domain_conf.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Thus is similar to commit e168bc8a did for virDomainDefPostParse...
Yes. That was actually my “inspiration”.
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.
Yes, do you have something better in mind? Maybe we can add an error label. While fixing it we can also adapt virDomainDefPostParse (were it’s, in my opinion, even more confusing since @ret is used multiple times).
+ + 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?
See my answer for patch 3.
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; }
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294