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

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? ________________________________________ From: Peter Krempa <pkrempa@redhat.com> Sent: Thursday, June 17, 2021 4:54 AM To: Michal Prívozník Cc: Douglas, William; libvir-list@redhat.com Subject: Re: [PATCH] ch_domain: Add handler for virDomainDeviceDefPostParseCallback 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@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.

On Fri, Jun 18, 2021 at 00:46:03 +0000, 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.

Well that exact goal can be achieved by exactly the same way, even with same code basically in the validation callback. The semantic difference between the two is that: - validation is done for any new XML - validation is (should be) re-tried on startup of an instance with an existing definition - code in vaidation callback must not fill any defaults or change anything - post-parse is done with all XML parsing (even existing configs) - post-parse may be retried in certain cases on startup but it's not the norm - post parse must never add any check which would fail with pre-existing XML documents You can move your code as-is as a validation callback. The benefit will be that any further addition to it doesn't need to be as closely scrutinized about breaking existing XML documents, while preserving both the compile-time addition checks as you've wanted.

In this case we'd really only be adding support so it wouldn't be increasing strictness on newer versions.

Well in fact yours is making the XML parser stricter, and even if you'd have existing configs on devel versions they would vanish. Granted this isn't making anythign stricter accross released versions. A further note is that this decreases the likelyhood that further addition making the parser stricter would land in the post-parse version and go unnoticed in libvirt.

I'm assuming that XML definitions with devices libvirt doesn't know about would fail to load.

Well, depending how you define them. They would fail in cases when the user asks for a XML schema validation while defining them. In that case if the XML contains anything not covered in the schema it's rejected. On the other hand without validation any new things are silently ignored.

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.

Yes that is true, any existing definition will not be impacted unless aditional code is added to the post-parse callback.

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.

I don't think this is true: static int virCHDomainDeviceDefPostParse(virDomainDeviceDef *dev, const virDomainDef *def G_GNUC_UNUSED, unsigned int parseFlags G_GNUC_UNUSED, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) vs. int qemuValidateDomainDeviceDef(const virDomainDeviceDef *dev, const virDomainDef *def, void *opaque, void *parseOpaque) Yes you don't get parseFlags but you apparently don't care.

Should I be going about this differently and I just misunderstood Daniel's advice?

No just move it as a validation callback, it's the same thing in the end.