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

Wednesday, 8 July 2020

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 and 
corresponding removals of free functions into one patch, then do the 
removal of the extra labels (in favor of directly returning) in a 
separate patch to make it easier to hand-verify / review. Here's a 
couple 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 single 
patch, except...

...

 Signed-off-by: Nicolas Brignone <nmbrignone(a)gmail.com&gt;
 ---
   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 be 
leftover from a time when we supported a compiler that required all 
declarations to be at top of scope, I think pretty much all of libvirts 
code 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 <laine(a)redhat.com&gt;

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 

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

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