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