On Thu, Jul 9, 2020 at 12:15 AM Laine Stump <laine@xxxxxxxxxx> wrote: > > 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 Makes sense, absolutely! > 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@xxxxxxxxx> > > --- > > 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 Thanks for that! > > > Reviewed-by: Laine Stump <laine@xxxxxxxxxx> > > > Congratulations on your first libvirt patch! :-) Thanks to you for reviewing so quickly and for the precise feedback. Regards! > > > > > 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 > >
