[libvirt] [PATCH] conf: remove parse code for long-extinct "<state devaddr='d:b:s'/>

Laine Stump posted 1 patch 6 days ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191009163915.9506-1-laine@redhat.com
src/conf/domain_conf.c | 76 +++++-------------------------------------
1 file changed, 8 insertions(+), 68 deletions(-)

[libvirt] [PATCH] conf: remove parse code for long-extinct "<state devaddr='d:b:s'/>

Posted by Laine Stump 6 days ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] conf: remove parse code for long-extinct "<state devaddr='d:b:s'/>

Posted by Ján Tomko 6 days ago
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list