[libvirt] [PATCH] domain: Remove controller/net address whitelists
Judging by how the whitelist has skewed quite far from the original error message, I think it's better to just drop these. If someone wants to revive this check I suggest implementing it on a per-HV driver basis with PostParse callbacks. --- src/conf/domain_conf.c | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d376a2c..ec14577 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7969,17 +7969,6 @@ virDomainControllerDefParseXML(xmlNodePtr node, break; } - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Controllers must use the 'pci' address type")); - goto error; - } - cleanup: ctxt->node = saved; VIR_FREE(typeStr); @@ -8670,19 +8659,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - /* XXX what about ISA/USB based NIC models - once we support - * them we should make sure address type is correct */ - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Network interfaces must use 'pci' address type")); - goto error; - } - switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: if (network == NULL) { -- 2.5.0
ping On 03/08/2016 11:39 AM, Cole Robinson wrote:
Judging by how the whitelist has skewed quite far from the original error message, I think it's better to just drop these.
If someone wants to revive this check I suggest implementing it on a per-HV driver basis with PostParse callbacks. --- src/conf/domain_conf.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d376a2c..ec14577 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7969,17 +7969,6 @@ virDomainControllerDefParseXML(xmlNodePtr node, break; }
- if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Controllers must use the 'pci' address type")); - goto error; - } - cleanup: ctxt->node = saved; VIR_FREE(typeStr); @@ -8670,19 +8659,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; }
- /* XXX what about ISA/USB based NIC models - once we support - * them we should make sure address type is correct */ - if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && - def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Network interfaces must use 'pci' address type")); - goto error; - } - switch (def->type) { case VIR_DOMAIN_NET_TYPE_NETWORK: if (network == NULL) {
On 03/08/2016 11:39 AM, Cole Robinson wrote:
Judging by how the whitelist has skewed quite far from the original error message, I think it's better to just drop these.
If someone wants to revive this check I suggest implementing it on a per-HV driver basis with PostParse callbacks.
Definitely the error messages for the current checks are incorrect, and certain types that are allowed here would actually be invalid for some hypervisors/machinetypes (the example you gave me in IRC was that vmware doesn't have virtio-mmio, nor do most qemu machinetypes), so you are correct that the proper place for at least some of the validation is in a post-parse callback. There is some value in the current validation, but it's dubious since any failure would have led to a misleading error message. Based on that I say ACK to this, but someone should put "improve address type validation in postparse" on their todo list :-)
On 03/18/2016 03:05 PM, Laine Stump wrote:
On 03/08/2016 11:39 AM, Cole Robinson wrote:
Judging by how the whitelist has skewed quite far from the original error message, I think it's better to just drop these.
If someone wants to revive this check I suggest implementing it on a per-HV driver basis with PostParse callbacks.
Definitely the error messages for the current checks are incorrect, and certain types that are allowed here would actually be invalid for some hypervisors/machinetypes (the example you gave me in IRC was that vmware doesn't have virtio-mmio, nor do most qemu machinetypes), so you are correct that the proper place for at least some of the validation is in a post-parse callback.
There is some value in the current validation, but it's dubious since any failure would have led to a misleading error message. Based on that I say ACK to this, but someone should put "improve address type validation in postparse" on their todo list :-)
Thanks, pushed. I'll add it to http://wiki.libvirt.org/page/BiteSizedTasks - Cole
participants (2)
-
Cole Robinson -
Laine Stump