[libvirt] [PATCH] conf: remove parse code for long-extinct "<state devaddr='d:b:s'/>
Back in July 2009, in the days before libvirt supported explicitly assigning a PCI address to every device, code was added to save the PCI addresses of hotplugged network, disk, and hostdevs in the domain status with this XML element: <status devaddr='domain:bus:slot'/> This was added in commits 4e21a95a, 01654107, in v0.7.0, and 0c5b7b93 in v0.7.1. Then just a few months later, in November 2009, The code that actually formatted the "devaddr='blah' into the status XML was removed by commit 1b0cce7d3 (which "Introduce[d] a standardized data structure for device addresses"). The code to *parse* the devaddr from the status was left in for backward compatibility though (it just parses it into the "standard" PCI address). At the time the devaddr attribute was added, a few other attributes already existed in the <status> element for network devices, and these were removed over time (I haven't checked the exact dates of this), but 10 years later, in libvirt v5.8.0, we *still* maintain code to parse <status devaddr='blah'/> from the domain status. In the meantime, even distros so old that we no longer support them in upstream libvirt are using a libvirt new enough that it doesn't ever write <status devaddr='blah'/> to the domain status XML. Since the only way a current libvirt would ever encounter this element would be if someone was upgrading directly from libvirt <=v0.7.5 with running guests, it seems safe to finally remove the code that parses it. Signed-off-by: Laine Stump <laine@redhat.com> --- The git commit ID's etc are probably not essential to have in the log message, but I did think it was interesting that we've been carrying this for so many years... src/conf/domain_conf.c | 76 +++++------------------------------------- 1 file changed, 8 insertions(+), 68 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a53cd6a725..423c242be9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7558,24 +7558,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, return ret; } -static int -virDomainParseLegacyDeviceAddress(char *devaddr, - virPCIDeviceAddressPtr pci) -{ - char *tmp; - - /* expected format: <domain>:<bus>:<slot> */ - if (/* domain */ - virStrToLong_ui(devaddr, &tmp, 16, &pci->domain) < 0 || *tmp != ':' || - /* bus */ - virStrToLong_ui(tmp + 1, &tmp, 16, &pci->bus) < 0 || *tmp != ':' || - /* slot */ - virStrToLong_ui(tmp + 1, NULL, 16, &pci->slot) < 0) - return -1; - - return 0; -} - static int virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, virDomainHostdevDefPtr def) @@ -7758,19 +7740,7 @@ virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, if (virPCIDeviceAddressParseXML(cur, addr) < 0) goto out; - } else if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) && - virXMLNodeNameEqual(cur, "state")) { - /* Legacy back-compat. Don't add any more attributes here */ - VIR_AUTOFREE(char *) devaddr = virXMLPropString(cur, "devaddr"); - if (devaddr && - virDomainParseLegacyDeviceAddress(devaddr, - &def->info->addr.pci) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to parse devaddr parameter '%s'"), - devaddr); - goto out; - } - def->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + } else if ((flags & VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES) && virXMLNodeNameEqual(cur, "origstates")) { virDomainHostdevOrigStatesPtr states = &def->origstates; @@ -9959,7 +9929,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_AUTOFREE(char *) sgio = NULL; VIR_AUTOFREE(char *) target = NULL; VIR_AUTOFREE(char *) bus = NULL; - VIR_AUTOFREE(char *) devaddr = NULL; VIR_AUTOFREE(char *) serial = NULL; VIR_AUTOFREE(char *) startupPolicy = NULL; VIR_AUTOFREE(char *) tray = NULL; @@ -10126,10 +10095,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->src->shared = true; } else if (virXMLNodeNameEqual(cur, "transient")) { def->transient = true; - } else if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) && - virXMLNodeNameEqual(cur, "state")) { - /* Legacy back-compat. Don't add any more attributes here */ - devaddr = virXMLPropString(cur, "devaddr"); } else if (!encryption && virXMLNodeNameEqual(cur, "encryption")) { /* If we've already parsed <source> and found an <encryption> child, @@ -10314,19 +10279,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } - if (devaddr) { - if (virDomainParseLegacyDeviceAddress(devaddr, - &def->info.addr.pci) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to parse devaddr parameter '%s'"), - devaddr); - goto error; - } - def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - } else { - if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, - flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) - goto error; + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, + flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) { + goto error; } if (startupPolicy) { @@ -11488,7 +11443,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_AUTOFREE(char *) str = NULL; VIR_AUTOFREE(char *) filter = NULL; VIR_AUTOFREE(char *) internal = NULL; - VIR_AUTOFREE(char *) devaddr = NULL; VIR_AUTOFREE(char *) mode = NULL; VIR_AUTOFREE(char *) linkstate = NULL; VIR_AUTOFREE(char *) addrtype = NULL; @@ -11669,10 +11623,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, filter = virXMLPropString(cur, "filter"); virHashFree(filterparams); filterparams = virNWFilterParseParamAttributes(cur); - } else if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) && - virXMLNodeNameEqual(cur, "state")) { - /* Legacy back-compat. Don't add any more attributes here */ - devaddr = virXMLPropString(cur, "devaddr"); } else if (virXMLNodeNameEqual(cur, "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ } else if (!actual && @@ -11725,20 +11675,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, def->mac_generated = true; } - if (devaddr) { - if (virDomainParseLegacyDeviceAddress(devaddr, - &def->info.addr.pci) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to parse devaddr parameter '%s'"), - devaddr); - goto error; - } - def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - } else { - if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, - flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT - | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) - goto error; + if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, + flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT + | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) { + goto error; } if (model != NULL && -- 2.21.0
Unmatched double quote in the commit summary On Wed, Oct 09, 2019 at 12:39:15PM -0400, Laine Stump wrote:
Back in July 2009, in the days before libvirt supported explicitly assigning a PCI address to every device, code was added to save the PCI addresses of hotplugged network, disk, and hostdevs in the domain status with this XML element:
<status devaddr='domain:bus:slot'/>
s/status/state/
This was added in commits 4e21a95a, 01654107, in v0.7.0, and 0c5b7b93 in v0.7.1.
Then just a few months later, in November 2009, The code that actually formatted the "devaddr='blah' into the status XML was removed by
Mismatched double quote
commit 1b0cce7d3 (which "Introduce[d] a standardized data structure for device addresses"). The code to *parse* the devaddr from the status was left in for backward compatibility though (it just parses it into the "standard" PCI address).
At the time the devaddr attribute was added, a few other attributes already existed in the <status> element for network devices, and these were removed over time (I haven't checked the exact dates of this), but 10 years later, in libvirt v5.8.0, we *still* maintain code to parse <status devaddr='blah'/> from the domain status.
s/status/state/
In the meantime, even distros so old that we no longer support them in upstream libvirt are using a libvirt new enough that it doesn't ever write <status devaddr='blah'/> to the domain status XML.
s/status/state/
Since the only way a current libvirt would ever encounter this element would be if someone was upgrading directly from libvirt <=v0.7.5 with
Please put a space between <= and v0.7.5
running guests, it seems safe to finally remove the code that parses it.
Signed-off-by: Laine Stump <laine@redhat.com> ---
The git commit ID's etc are probably not essential to have in the log message, but I did think it was interesting that we've been carrying this for so many years...
Having the commit ID's there shows that you actually looked at the history instead of just digging in your memory.
src/conf/domain_conf.c | 76 +++++------------------------------------- 1 file changed, 8 insertions(+), 68 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Laine Stump