Re: [PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback

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, }

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(a)intel.com&gt; >> --- >> 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, }