Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.

Laine Stump Wed, 08 Jul 2020 20:16:17 -0700

On 7/8/20 4:19 PM, Nicolas Brignone wrote:

  All pointers to virXMLPropString() use g_autofree.



I changed the summary line like this, to be more precise:


conf: use g_autofree for all pointers to virXMLPropString() in device_conf.c

All modified functions are similar, in all cases "cleanup" label is removed,
along with all the "goto" calls.

I've been advised in the recent past to put the g_autofree additions andcorresponding removals of free functions into one patch, then do theremoval of the extra labels (in favor of directly returning) in aseparate patch to make it easier to hand-verify / review. Here's acouple recent examples:



https://www.redhat.com/archives/libvir-list/2020-July/msg00317.html

In your case the changes are few enough that I'm okay with it a singlepatch, except...


Signed-off-by: Nicolas Brignone <nmbrign...@gmail.com>
---
  src/conf/device_conf.c | 183 +++++++++++++----------------------------
  1 file changed, 56 insertions(+), 127 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index 7d48a3f..9fa6141 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -208,45 +208,43 @@ int
  virPCIDeviceAddressParseXML(xmlNodePtr node,
                              virPCIDeviceAddressPtr addr)
  {
-    char *domain, *slot, *bus, *function, *multi;
      xmlNodePtr cur;
      xmlNodePtr zpci = NULL;
-    int ret = -1;

memset(addr, 0, sizeof(*addr));- domain = virXMLPropString(node, "domain");

-    bus      = virXMLPropString(node, "bus");
-    slot     = virXMLPropString(node, "slot");
-    function = virXMLPropString(node, "function");
-    multi    = virXMLPropString(node, "multifunction");
+    g_autofree char *domain   = virXMLPropString(node, "domain");
+    g_autofree char *bus      = virXMLPropString(node, "bus");
+    g_autofree char *slot     = virXMLPropString(node, "slot");
+    g_autofree char *function = virXMLPropString(node, "function");
+    g_autofree char *multi    = virXMLPropString(node, "multifunction");

... you've modified it so that local variables are being declared*below* a line of executable code rather than at the top of the block.Although I don't see anything in the coding style document(https://libvirt.org/coding-style.html) about it, and it may just beleftover from a time when we supported a compiler that required alldeclarations to be at top of scope, I think pretty much all of libvirtscode declares all local variables at the top of the scope.


That's simple enough for me to just fixup before pushing, so


Reviewed-by: Laine Stump <la...@redhat.com>


Congratulations on your first libvirt patch! :-)

if (domain &&

          virStrToLong_uip(domain, NULL, 0, &addr->domain) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'domain' attribute"));
-        goto cleanup;
+        return -1;
      }

if (bus &&

          virStrToLong_uip(bus, NULL, 0, &addr->bus) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'bus' attribute"));
-        goto cleanup;
+        return -1;
      }

if (slot &&

          virStrToLong_uip(slot, NULL, 0, &addr->slot) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'slot' attribute"));
-        goto cleanup;
+        return -1;
      }

if (function &&

          virStrToLong_uip(function, NULL, 0, &addr->function) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'function' attribute"));
-        goto cleanup;
+        return -1;
      }

if (multi &&

@@ -254,11 +252,11 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                         _("Unknown value '%s' for <address> 'multifunction' 
attribute"),
                         multi);
-        goto cleanup;
+        return -1;

}

      if (!virPCIDeviceAddressIsEmpty(addr) && 
!virPCIDeviceAddressIsValid(addr, true))
-        goto cleanup;
+        return -1;

cur = node->children;

      while (cur) {
@@ -270,17 +268,9 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
      }

if (zpci && virZPCIDeviceAddressParseXML(zpci, addr) < 0)

-        goto cleanup;
+        return -1;

- ret = 0;

-
- cleanup:
-    VIR_FREE(domain);
-    VIR_FREE(bus);
-    VIR_FREE(slot);
-    VIR_FREE(function);
-    VIR_FREE(multi);
-    return ret;
+    return 0;
  }

void

@@ -309,187 +299,149 @@ int
  virDomainDeviceCCWAddressParseXML(xmlNodePtr node,
                                    virDomainDeviceCCWAddressPtr addr)
  {
-    int   ret = -1;
-    char *cssid;
-    char *ssid;
-    char *devno;
-
      memset(addr, 0, sizeof(*addr));

- cssid = virXMLPropString(node, "cssid");

-    ssid = virXMLPropString(node, "ssid");
-    devno = virXMLPropString(node, "devno");
+    g_autofree char *cssid = virXMLPropString(node, "cssid");
+    g_autofree char *ssid = virXMLPropString(node, "ssid");
+    g_autofree char *devno = virXMLPropString(node, "devno");

if (cssid && ssid && devno) {

          if (cssid &&
              virStrToLong_uip(cssid, NULL, 0, &addr->cssid) < 0) {
              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                             _("Cannot parse <address> 'cssid' attribute"));
-            goto cleanup;
+            return -1;
          }
          if (ssid &&
              virStrToLong_uip(ssid, NULL, 0, &addr->ssid) < 0) {
              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                             _("Cannot parse <address> 'ssid' attribute"));
-            goto cleanup;
+            return -1;
          }
          if (devno &&
              virStrToLong_uip(devno, NULL, 0, &addr->devno) < 0) {
              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                             _("Cannot parse <address> 'devno' attribute"));
-            goto cleanup;
+            return -1;
          }
          if (!virDomainDeviceCCWAddressIsValid(addr)) {
              virReportError(VIR_ERR_INTERNAL_ERROR,
                             _("Invalid specification for virtio ccw"
                               " address: cssid='%s' ssid='%s' devno='%s'"),
                             cssid, ssid, devno);
-            goto cleanup;
+            return -1;
          }
          addr->assigned = true;
      } else if (cssid || ssid || devno) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Invalid partial specification for virtio ccw"
                           " address"));
-        goto cleanup;
+        return -1;
      }

- ret = 0;

-
- cleanup:
-    VIR_FREE(cssid);
-    VIR_FREE(ssid);
-    VIR_FREE(devno);
-    return ret;
+    return 0;
  }

int

  virDomainDeviceDriveAddressParseXML(xmlNodePtr node,
                                      virDomainDeviceDriveAddressPtr addr)
  {
-    char *bus, *unit, *controller, *target;
-    int ret = -1;
-
      memset(addr, 0, sizeof(*addr));

- controller = virXMLPropString(node, "controller");

-    bus = virXMLPropString(node, "bus");
-    target = virXMLPropString(node, "target");
-    unit = virXMLPropString(node, "unit");
+    g_autofree char *controller = virXMLPropString(node, "controller");
+    g_autofree char *bus = virXMLPropString(node, "bus");
+    g_autofree char *target = virXMLPropString(node, "target");
+    g_autofree char *unit = virXMLPropString(node, "unit");

if (controller &&

          virStrToLong_uip(controller, NULL, 10, &addr->controller) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'controller' attribute"));
-        goto cleanup;
+        return -1;
      }

if (bus &&

          virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'bus' attribute"));
-        goto cleanup;
+        return -1;
      }

if (target &&

          virStrToLong_uip(target, NULL, 10, &addr->target) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'target' attribute"));
-        goto cleanup;
+        return -1;
      }

if (unit &&

          virStrToLong_uip(unit, NULL, 10, &addr->unit) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'unit' attribute"));
-        goto cleanup;
+        return -1;
      }

- ret = 0;

-
- cleanup:
-    VIR_FREE(controller);
-    VIR_FREE(bus);
-    VIR_FREE(target);
-    VIR_FREE(unit);
-    return ret;
+    return 0;
  }

int

  virDomainDeviceVirtioSerialAddressParseXML(xmlNodePtr node,
                                             
virDomainDeviceVirtioSerialAddressPtr addr)
  {
-    char *controller, *bus, *port;
-    int ret = -1;
-
      memset(addr, 0, sizeof(*addr));

- controller = virXMLPropString(node, "controller");

-    bus = virXMLPropString(node, "bus");
-    port = virXMLPropString(node, "port");
+    g_autofree char *controller = virXMLPropString(node, "controller");
+    g_autofree char *bus = virXMLPropString(node, "bus");
+    g_autofree char *port = virXMLPropString(node, "port");

if (controller &&

          virStrToLong_uip(controller, NULL, 10, &addr->controller) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'controller' attribute"));
-        goto cleanup;
+        return -1;
      }

if (bus &&

          virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'bus' attribute"));
-        goto cleanup;
+        return -1;
      }

if (port &&

          virStrToLong_uip(port, NULL, 10, &addr->port) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'port' attribute"));
-        goto cleanup;
+        return -1;
      }

- ret = 0;

-
- cleanup:
-    VIR_FREE(controller);
-    VIR_FREE(bus);
-    VIR_FREE(port);
-    return ret;
+    return 0;
  }

int

  virDomainDeviceCcidAddressParseXML(xmlNodePtr node,
                                     virDomainDeviceCcidAddressPtr addr)
  {
-    char *controller, *slot;
-    int ret = -1;
-
      memset(addr, 0, sizeof(*addr));

- controller = virXMLPropString(node, "controller");

-    slot = virXMLPropString(node, "slot");
+    g_autofree char *controller = virXMLPropString(node, "controller");
+    g_autofree char *slot = virXMLPropString(node, "slot");

if (controller &&

          virStrToLong_uip(controller, NULL, 10, &addr->controller) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'controller' attribute"));
-        goto cleanup;
+        return -1;
      }

if (slot &&

          virStrToLong_uip(slot, NULL, 10, &addr->slot) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'slot' attribute"));
-        goto cleanup;
+        return -1;
      }

- ret = 0;

-
- cleanup:
-    VIR_FREE(controller);
-    VIR_FREE(slot);
-    return ret;
+    return 0;
  }

static int

@@ -519,57 +471,41 @@ int
  virDomainDeviceUSBAddressParseXML(xmlNodePtr node,
                                    virDomainDeviceUSBAddressPtr addr)
  {
-    char *port, *bus;
-    int ret = -1;

memset(addr, 0, sizeof(*addr));- port = virXMLPropString(node, "port");

-    bus = virXMLPropString(node, "bus");
+    g_autofree char *port = virXMLPropString(node, "port");
+    g_autofree char *bus = virXMLPropString(node, "bus");

if (port && virDomainDeviceUSBAddressParsePort(addr, port) < 0)

-        goto cleanup;
+        return -1;

if (bus &&

          virStrToLong_uip(bus, NULL, 10, &addr->bus) < 0) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Cannot parse <address> 'bus' attribute"));
-        goto cleanup;
+        return -1;
      }
-
-    ret = 0;
-
- cleanup:
-    VIR_FREE(bus);
-    VIR_FREE(port);
-    return ret;
+    return 0;
  }

int

  virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node,
                                        virDomainDeviceSpaprVioAddressPtr addr)
  {
-    char *reg;
-    int ret;
-
      memset(addr, 0, sizeof(*addr));

- reg = virXMLPropString(node, "reg");

+    g_autofree char *reg = virXMLPropString(node, "reg");
      if (reg) {
          if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) {
              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                             _("Cannot parse <address> 'reg' attribute"));
-            ret = -1;
-            goto cleanup;
+            return -1;
          }

addr->has_reg = true;

      }
-
-    ret = 0;
- cleanup:
-    VIR_FREE(reg);
-    return ret;
+    return 0;
  }

bool

@@ -604,19 +540,17 @@ int
  virInterfaceLinkParseXML(xmlNodePtr node,
                           virNetDevIfLinkPtr lnk)
  {
-    int ret = -1;
-    char *stateStr, *speedStr;
      int state;

- stateStr = virXMLPropString(node, "state");

-    speedStr = virXMLPropString(node, "speed");
+    g_autofree char *stateStr = virXMLPropString(node, "state");
+    g_autofree char *speedStr = virXMLPropString(node, "speed");

if (stateStr) {

          if ((state = virNetDevIfStateTypeFromString(stateStr)) < 0) {
              virReportError(VIR_ERR_XML_ERROR,
                             _("unknown link state: %s"),
                             stateStr);
-            goto cleanup;
+            return -1;
          }
          lnk->state = state;
      }
@@ -626,14 +560,9 @@ virInterfaceLinkParseXML(xmlNodePtr node,
          virReportError(VIR_ERR_XML_ERROR,
                         _("Unable to parse link speed: %s"),
                         speedStr);
-        goto cleanup;
+        return -1;
      }
-
-    ret = 0;
- cleanup:
-    VIR_FREE(stateStr);
-    VIR_FREE(speedStr);
-    return ret;
+    return 0;
  }

int

Re: [libvirt PATCH] All pointers to virXMLPropString() use g_autofree.

Reply via email to