[PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback

[Michal Prívozník]

On 6/18/21 2:46 AM, Douglas, William wrote:
> Ick sorry for the malformed mail...
> 
> On 6/17/21 10:33 AM, Michal Prívozník wrote:
>> On 6/17/21 9:00 AM, Peter Krempa wrote:
>>> On Wed, Jun 16, 2021 at 21:16:01 -0700, William Douglas wrote:
>>>> Instead of trying to match devices passed in based on the monitor
>>>> detecting the number of devices that were used in the domain
>>>> definition, use the devicesPostParseCallback to evaluate if
>>>> unsupported devices are used.
>>>>
>>>> This allows the compiler to detect when new device types are added
>>>> that need to be checked.
>>>>
>>>> Signed-off-by: William Douglas <william douglas intel com>
>>>> ---
>>>>  src/ch/ch_domain.c  | 121 +++++++++++++++++++++++++++++++++++++++++++
>>>>  src/ch/ch_monitor.c | 122 --------------------------------------------
>>>>  2 files changed, 121 insertions(+), 122 deletions(-)
>>>
>>> Note that putting stuff into the post-parse callback will result in the
>>> failure/rejection happening already at XML parsing time.
>>>
>>> I hope that's what you intended.
>>>
>>> Generally such a change would not be acceptable because strictening the
>>> parser means we would fail at loading already defined configuration XMLs
>>> e.g. at libvirtd restart.
>>>
>>> In this particular instance it's okay because the cloud hypervisor code
>>> was not yet released.
>>>
>>> Also note that the addition of the cloud hypervisor driver was not yet
>>> documented in NEWS.rst.
>>
>> However, I would argue that this should go into validation rather than
>> post parse. To me, XML parsing happens in these steps:
>>
>> 1) actual parsing and filling internal structures (e.g. using XPATHs,
>> xmlPropString, etc.)
>>
>> 2) post parse (filling in missing info, e.g. defaults)
>>
>> 3) validation (checking whether definition makes sense).
>>
>>
>> And this patch is performing validation. So I think we need to set
>> different member (taken from QEMU driver):
>>
>>
>> virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = {
>>     .deviceValidateCallback = qemuValidateDomainDeviceDef,
>> }
>>
> 
> I tried to follow advice from Daniel (on
> https://listman.redhat.com/archives/libvir-list/2021-June/msg00084.html).
> The goal I had in mind from his last comment was that the cloud-hypervisor driver would catch
> new devices due to missing items from the enum in the switch when they are added to libvirt.
> 
> In this case we'd really only be adding support so it wouldn't be increasing strictness on newer
> versions. I'm assuming that XML definitions with devices libvirt doesn't know about would fail to
> load. Then if a new libvirt is loaded after restart with new hardware support (but cloud-hypervisor
> doesn't support it) existing configurations wouldn't be impacted.
> 
> This is definitely validation but the validation call's type signature doesn't seem to align very
> closely to the use case I was trying to fill. Should I be going about this differently and I just
> misunderstood Daniel's advice?
> 

No, Dan's point is correct and you are doing the right thing, almost :-)
What Peter and I are talking about is to put the check into different
phase of XML parsing.

Here is a snippet of stacktrace of XML parsing:

virDomainDefineXMLFlags
chDomainDefineXMLFlags
virDomainDefParseString
virDomainDefParse
virDomainDefParseNode

Now, this last function consists of those three steps I was talking
about earlier:

virDomainDef *
virDomainDefParseNode(...)
{
    ...
    if (!(def = virDomainDefParseXML(xml, ctxt, xmlopt, flags)))
        return NULL;

    /* callback to fill driver specific domain aspects */
    if (virDomainDefPostParse(def, flags, xmlopt, parseOpaque) < 0)
        return NULL;

    /* validate configuration */
    if (virDomainDefValidate(def, flags, xmlopt, parseOpaque) < 0)
        return NULL;
    ...
}


The devicesPostParseCallback will be called from virDomainDefPostParse()
and the deviceValidateCallback will be called from
virDomainDefValidate(). Thus moving the check from the former to the
latter does not make situation worse. I mean, the
virDomainDefParseNode() would still fail.

However, there is a strong reason why we want this kind of checks to
live in validation step. When libvirtd starts up it parses XMLs of
defined domains from /etc/libvirt/..., but it does so with @flags having
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE set. This means, that as we tighten
some checks in validation step, we don't fail to reload a defined domain
XML on libvirtd restart/upgrade.

Long story short, Dan's suggestion was correct from conceptual POV. But
slightly less so from placement POV.

Michal