[PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback

[Peter Krempa]

On Thu, Jun 17, 2021 at 10:33:12 +0200, 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 at 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 concur. One additional reason is that seeing such code in the
post-parse callback may motivate further additions which would no-longer
comply with the rule we have for post-parse stuff, so validation
callback is the place also for further extensions.