On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan <jferlan@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
>> ---
>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
