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

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