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

[Marc Hartmayer]

On Sat, Sep 29, 2018 at 05:35 PM +0200, John Ferlan <jferlan at 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 at linux.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy at 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