[libvirt PATCH] All pointers to virXMLPropString() use g_autofree.
Laine Stump
laine at redhat.com
Thu Jul 9 03:15:27 UTC 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 at 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 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 at 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
More information about the libvir-list
mailing list