src/ch/ch_domain.c | 121 +++++++++++++++++++++++++++++++++++++++++++ src/ch/ch_monitor.c | 122 -------------------------------------------- 2 files changed, 121 insertions(+), 122 deletions(-)
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(-)
diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
index f9a6f3f31d..02ca5ea06c 100644
--- a/src/ch/ch_domain.c
+++ b/src/ch/ch_domain.c
@@ -197,7 +197,128 @@ virCHDomainDefPostParse(virDomainDef *def,
return 0;
}
+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)
+{
+ int ret = -1;
+
+ switch ((virDomainDeviceType)dev->type) {
+ case VIR_DOMAIN_DEVICE_DISK:
+ ret = 0;
+ break;
+ case VIR_DOMAIN_DEVICE_LEASE:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support lease"));
+ break;
+ case VIR_DOMAIN_DEVICE_FS:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support fs"));
+ break;
+ case VIR_DOMAIN_DEVICE_NET:
+ ret = 0;
+ break;
+ case VIR_DOMAIN_DEVICE_INPUT:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support input"));
+ break;
+ case VIR_DOMAIN_DEVICE_SOUND:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support sound"));
+ break;
+ case VIR_DOMAIN_DEVICE_VIDEO:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support video"));
+ break;
+ case VIR_DOMAIN_DEVICE_HOSTDEV:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support hostdev"));
+ break;
+ case VIR_DOMAIN_DEVICE_WATCHDOG:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support watchdog"));
+ break;
+ case VIR_DOMAIN_DEVICE_CONTROLLER:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support controller"));
+ break;
+ case VIR_DOMAIN_DEVICE_GRAPHICS:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support graphics"));
+ break;
+ case VIR_DOMAIN_DEVICE_HUB:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support hub"));
+ break;
+ case VIR_DOMAIN_DEVICE_REDIRDEV:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support redirdev"));
+ break;
+ case VIR_DOMAIN_DEVICE_SMARTCARD:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support smartcard"));
+ break;
+ case VIR_DOMAIN_DEVICE_CHR:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support chr"));
+ break;
+ case VIR_DOMAIN_DEVICE_MEMBALLOON:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support memballoon"));
+ break;
+ case VIR_DOMAIN_DEVICE_NVRAM:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support nvram"));
+ break;
+ case VIR_DOMAIN_DEVICE_RNG:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support rng"));
+ break;
+ case VIR_DOMAIN_DEVICE_SHMEM:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support shmem"));
+ break;
+ case VIR_DOMAIN_DEVICE_TPM:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support tpm"));
+ break;
+ case VIR_DOMAIN_DEVICE_PANIC:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support panic"));
+ break;
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ ret = 0;
+ break;
+ case VIR_DOMAIN_DEVICE_IOMMU:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support iommu"));
+ break;
+ case VIR_DOMAIN_DEVICE_VSOCK:
+ ret = 0;
+ break;
+ case VIR_DOMAIN_DEVICE_AUDIO:
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Cloud-Hypervisor doesn't support audio"));
+ break;
+ case VIR_DOMAIN_DEVICE_NONE:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("unexpected VIR_DOMAIN_DEVICE_NONE"));
+ break;
+
+ case VIR_DOMAIN_DEVICE_LAST:
+ default:
+ virReportEnumRangeError(virDomainDeviceType, dev->type);
+ break;
+ }
+
+ return ret;
+}
+
virDomainDefParserConfig virCHDriverDomainDefParserConfig = {
.domainPostParseBasicCallback = virCHDomainDefPostParseBasic,
.domainPostParseCallback = virCHDomainDefPostParse,
+ .devicesPostParseCallback = virCHDomainDeviceDefPostParse,
};
diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c
index 87520a2639..1648d05017 100644
--- a/src/ch/ch_monitor.c
+++ b/src/ch/ch_monitor.c
@@ -359,125 +359,6 @@ virCHMonitorBuildNetsJson(virJSONValue *content, virDomainDef *vmdef)
return -1;
}
-static int
-virCHMonitorDetectUnsupportedDevices(virDomainDef *vmdef)
-{
- int ret = 0;
-
- if (vmdef->ngraphics > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support graphics"));
- ret = 1;
- }
- if (vmdef->ncontrollers > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support controllers"));
- ret = 1;
- }
- if (vmdef->nfss > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support fss"));
- ret = 1;
- }
- if (vmdef->ninputs > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support inputs"));
- ret = 1;
- }
- if (vmdef->nsounds > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support sounds"));
- ret = 1;
- }
- if (vmdef->naudios > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support audios"));
- ret = 1;
- }
- if (vmdef->nvideos > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support videos"));
- ret = 1;
- }
- if (vmdef->nhostdevs > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support hostdevs"));
- ret = 1;
- }
- if (vmdef->nredirdevs > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support redirdevs"));
- ret = 1;
- }
- if (vmdef->nsmartcards > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support smartcards"));
- ret = 1;
- }
- if (vmdef->nserials > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support serials"));
- ret = 1;
- }
- if (vmdef->nparallels > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support parallels"));
- ret = 1;
- }
- if (vmdef->nchannels > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support channels"));
- ret = 1;
- }
- if (vmdef->nconsoles > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support consoles"));
- ret = 1;
- }
- if (vmdef->nleases > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support leases"));
- ret = 1;
- }
- if (vmdef->nhubs > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support hubs"));
- ret = 1;
- }
- if (vmdef->nseclabels > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support seclabels"));
- ret = 1;
- }
- if (vmdef->nrngs > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support rngs"));
- ret = 1;
- }
- if (vmdef->nshmems > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support shmems"));
- ret = 1;
- }
- if (vmdef->nmems > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support mems"));
- ret = 1;
- }
- if (vmdef->npanics > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support panics"));
- ret = 1;
- }
- if (vmdef->nsysinfo > 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("Cloud-Hypervisor doesn't support sysinfo"));
- ret = 1;
- }
-
- return ret;
-}
-
static int
virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr)
{
@@ -490,9 +371,6 @@ virCHMonitorBuildVMJson(virDomainDef *vmdef, char **jsonstr)
goto cleanup;
}
- if (virCHMonitorDetectUnsupportedDevices(vmdef))
- goto cleanup;
-
if (virCHMonitorBuildCPUJson(content, vmdef) < 0)
goto cleanup;
--
2.31.1
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.
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 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 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.
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?
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.
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
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(-)
>
> diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c
> index f9a6f3f31d..02ca5ea06c 100644
> --- a/src/ch/ch_domain.c
> +++ b/src/ch/ch_domain.c
> @@ -197,7 +197,128 @@ virCHDomainDefPostParse(virDomainDef *def,
> return 0;
> }
>
> +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)
> +{
> + int ret = -1;
> +
> + switch ((virDomainDeviceType)dev->type) {
> + case VIR_DOMAIN_DEVICE_DISK:
> + ret = 0;
> + break;
> + case VIR_DOMAIN_DEVICE_LEASE:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support lease"));
> + break;
> + case VIR_DOMAIN_DEVICE_FS:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support fs"));
> + break;
> + case VIR_DOMAIN_DEVICE_NET:
> + ret = 0;
> + break;
> + case VIR_DOMAIN_DEVICE_INPUT:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support input"));
> + break;
> + case VIR_DOMAIN_DEVICE_SOUND:
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Cloud-Hypervisor doesn't support sound"));
> + break;
To further simplify the code you can also do the following (just an
example):
case VIR_DOMAIN_DEVICE_VIDEO:
case VIR_DOMAIN_DEVICE_HOSTDEV:
case VIR_DOMAIN_DEVICE_WATCHDOG:
case VIR_DOMAIN_DEVICE_CONTROLLER:
case VIR_DOMAIN_DEVICE_GRAPHICS:
case VIR_DOMAIN_DEVICE_HUB:
case VIR_DOMAIN_DEVICE_REDIRDEV:
case VIR_DOMAIN_DEVICE_SMARTCARD:
case VIR_DOMAIN_DEVICE_CHR:
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Cloud-Hypervisor doesn't support '%s' device"),
virDomainDeviceTypeToString(dev->type));
return -1;
Also note that in validation callbacks any invalid configuration should
direclty return -1 to short circuit the execution so that we don't need
to have a 'ret' variable. It makes the code easier to read.
© 2016 - 2024 Red Hat, Inc.