There's one 'return' in the middle of the function body. It's
very easy to miss and so it makes adding new code harder. Also
the function doesn't follow our style 100%.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/conf/domain_conf.c | 174 ++++++++++++++++++++++++++-----------------------
1 file changed, 93 insertions(+), 81 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 87192eb2d..0bc2e2f94 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6303,6 +6303,93 @@ virDomainDeviceDimmAddressParseXML(xmlNodePtr node,
}
+static int
+virDomainDeviceAddressParseXML(xmlNodePtr address,
+ virDomainDeviceInfoPtr info)
+{
+ int ret = -1;
+ char *type = virXMLPropString(address, "type");
+
+ if (type) {
+ if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown address type '%s'"), type);
+ goto cleanup;
+ }
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("No type specified for device address"));
+ goto cleanup;
+ }
+
+ switch ((virDomainDeviceAddressType) info->type) {
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
+ if (virPCIDeviceAddressParseXML(address, &info->addr.pci) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
+ if (virDomainDeviceDriveAddressParseXML(address, &info->addr.drive) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
+ if (virDomainDeviceVirtioSerialAddressParseXML
+ (address, &info->addr.vioserial) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
+ if (virDomainDeviceCcidAddressParseXML(address, &info->addr.ccid) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
+ if (virDomainDeviceUSBAddressParseXML(address, &info->addr.usb) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
+ if (virDomainDeviceSpaprVioAddressParseXML(address, &info->addr.spaprvio) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
+ if (virDomainDeviceCCWAddressParseXML
+ (address, &info->addr.ccw) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
+ if (virDomainDeviceISAAddressParseXML(address, &info->addr.isa) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("virtio-s390 bus doesn't have an address"));
+ goto cleanup;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
+ if (virDomainDeviceDimmAddressParseXML(address, &info->addr.dimm) < 0)
+ goto cleanup;
+ break;
+
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
+ break;
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(type);
+ return ret;
+}
+
+
/* Parse the XML definition for a device address
* @param node XML nodeset to parse for device address definition
*/
@@ -6319,6 +6406,7 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
xmlNodePtr boot = NULL;
xmlNodePtr rom = NULL;
char *type = NULL;
+ char *rombar = NULL;
int ret = -1;
virDomainDeviceInfoClear(info);
@@ -6364,102 +6452,26 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
}
if (rom) {
- char *rombar = virXMLPropString(rom, "bar");
- if (rombar &&
+ if ((rombar = virXMLPropString(rom, "bar")) &&
((info->rombar = virTristateSwitchTypeFromString(rombar)) <= 0)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown rom bar value '%s'"), rombar);
- VIR_FREE(rombar);
goto cleanup;
}
- VIR_FREE(rombar);
info->romfile = virXMLPropString(rom, "file");
}
- if (!address)
- return 0;
-
- type = virXMLPropString(address, "type");
-
- if (type) {
- if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown address type '%s'"), type);
- goto cleanup;
- }
- } else {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("No type specified for device address"));
- goto cleanup;
- }
-
- switch ((virDomainDeviceAddressType) info->type) {
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
- if (virPCIDeviceAddressParseXML(address, &info->addr.pci) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
- if (virDomainDeviceDriveAddressParseXML(address, &info->addr.drive) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL:
- if (virDomainDeviceVirtioSerialAddressParseXML
- (address, &info->addr.vioserial) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID:
- if (virDomainDeviceCcidAddressParseXML(address, &info->addr.ccid) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB:
- if (virDomainDeviceUSBAddressParseXML(address, &info->addr.usb) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO:
- if (virDomainDeviceSpaprVioAddressParseXML(address, &info->addr.spaprvio) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW:
- if (virDomainDeviceCCWAddressParseXML
- (address, &info->addr.ccw) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO:
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_ISA:
- if (virDomainDeviceISAAddressParseXML(address, &info->addr.isa) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390:
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("virtio-s390 bus doesn't have an address"));
+ if (address &&
+ virDomainDeviceAddressParseXML(address, info) < 0)
goto cleanup;
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
- if (virDomainDeviceDimmAddressParseXML(address, &info->addr.dimm) < 0)
- goto cleanup;
- break;
-
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
- case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST:
- break;
- }
ret = 0;
-
cleanup:
- if (ret == -1)
+ if (ret < 0)
VIR_FREE(info->alias);
VIR_FREE(type);
+ VIR_FREE(rombar);
return ret;
}
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 03, 2017 at 12:58:57PM +0200, Michal Privoznik wrote:
>There's one 'return' in the middle of the function body. It's
>very easy to miss and so it makes adding new code harder. Also
>the function doesn't follow our style 100%.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/conf/domain_conf.c | 174 ++++++++++++++++++++++++++-----------------------
> 1 file changed, 93 insertions(+), 81 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 87192eb2d..0bc2e2f94 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -6364,102 +6452,26 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
> }
>
> if (rom) {
>- char *rombar = virXMLPropString(rom, "bar");
>- if (rombar &&
>+ if ((rombar = virXMLPropString(rom, "bar")) &&
If this is what you meant by "follow our coding style 100%", then I
think you are wrong since this (IMHO bad) style is not explicitly
mentioned, only (mis)used once. But I'm not fighting over it since I
know more people prefer less readable code that saves one or two lines =)
ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 10/03/2017 01:52 PM, Martin Kletzander wrote:
> On Tue, Oct 03, 2017 at 12:58:57PM +0200, Michal Privoznik wrote:
>> There's one 'return' in the middle of the function body. It's
>> very easy to miss and so it makes adding new code harder. Also
>> the function doesn't follow our style 100%.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/conf/domain_conf.c | 174
>> ++++++++++++++++++++++++++-----------------------
>> 1 file changed, 93 insertions(+), 81 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 87192eb2d..0bc2e2f94 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6364,102 +6452,26 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
>> }
>>
>> if (rom) {
>> - char *rombar = virXMLPropString(rom, "bar");
>> - if (rombar &&
>> + if ((rombar = virXMLPropString(rom, "bar")) &&
>
> If this is what you meant by "follow our coding style 100%", then I
> think you are wrong since this (IMHO bad) style is not explicitly
> mentioned, only (mis)used once. But I'm not fighting over it since I
> know more people prefer less readable code that saves one or two lines =)
No, I was referring to return in the middle of the function. And then to
freeing variables before goto statements, and not freeing them under the
cleanup label. Whatever,
>
> ACK
this is important to me :-)
Thanks,
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.