[libvirt] [PATCH 1/3] conf: Fix error flow in virDomainPCIAddressEnsureAddr()

[Andrea Bolognani]

This avoids setting 'ret' multiple times, which will result
in errors being masked if the first operation fails but the
second one succeeds.

Introduced-by: f183b87fc1dbcc6446ac3c1cef9cdd345b9725fb
Spotted-by: Coverity
Reported-by: John Ferlan <***@redhat.com>
Signed-off-by: Andrea Bolognani <***@redhat.com>
---
src/conf/domain_addr.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 3e1d767e4f..cc9ea82a33 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -940,15 +940,21 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs,
addrStr, flags, true))
goto cleanup;

- ret = virDomainPCIAddressReserveAddrInternal(addrs, &dev->addr.pci,
- flags, dev->isolationGroup,
- true);
+ if (virDomainPCIAddressReserveAddrInternal(addrs, &dev->addr.pci,
+ flags, dev->isolationGroup,
+ true) < 0) {
+ goto cleanup;
+ }
} else {
- ret = virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1);
+ if (virDomainPCIAddressReserveNextAddr(addrs, dev, flags, -1) < 0)
+ goto cleanup;
}

dev->addr.pci.extFlags = dev->pciAddrExtFlags;
- ret = virDomainPCIAddressExtensionEnsureAddr(addrs, &dev->addr.pci);
+ if (virDomainPCIAddressExtensionEnsureAddr(addrs, &dev->addr.pci) < 0)
+ goto cleanup;
+
+ ret = 0;

cleanup:
VIR_FREE(addrStr);

[Andrea Bolognani]

In many cases, an early exit from a function would cause
memory allocated by local virBuffer instances not to be
released.

Provide proper cleanup paths to solve the issue.

Signed-off-by: Andrea Bolognani <***@redhat.com>
---
src/conf/domain_conf.c | 137 ++++++++++++++++++++++++++++++-----------
1 file changed, 100 insertions(+), 37 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c3dbba6919..1d04cac104 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -24353,13 +24353,14 @@ virDomainControllerDefFormat(virBufferPtr buf,
const char *model = NULL;
const char *modelName = NULL;
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

virBufferSetChildIndent(&childBuf, buf);

if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected controller type %d"), def->type);
- return -1;
+ goto cleanup;
}

if (def->model != -1) {
@@ -24368,7 +24369,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
if (!model) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected model type %d"), def->model);
- return -1;
+ goto cleanup;
}
}

@@ -24432,7 +24433,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected model name value %d"),
def->opts.pciopts.modelName);
- return -1;
+ goto cleanup;
}
virBufferAsprintf(&childBuf, "<model name='%s'/>\n", modelName);
}
@@ -24483,7 +24484,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
}

if (virBufferCheckError(&childBuf) < 0)
- return -1;
+ goto cleanup;

if (virBufferUse(&childBuf)) {
virBufferAddLit(buf, ">\n");
@@ -24493,6 +24494,11 @@ virDomainControllerDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&childBuf);
+
return 0;
}

@@ -24523,17 +24529,18 @@ virDomainFSDefFormat(virBufferPtr buf,
const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy);
const char *src = def->src->path;
virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected filesystem type %d"), def->type);
- return -1;
+ goto cleanup;
}

if (!accessmode) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected accessmode %d"), def->accessmode);
- return -1;
+ goto cleanup;
}


@@ -24557,7 +24564,7 @@ virDomainFSDefFormat(virBufferPtr buf,
virDomainVirtioOptionsFormat(&driverBuf, def->virtio);

if (virBufferCheckError(&driverBuf) < 0)
- return -1;
+ goto cleanup;

if (virBufferUse(&driverBuf)) {
virBufferAddLit(buf, "<driver");
@@ -24617,7 +24624,13 @@ virDomainFSDefFormat(virBufferPtr buf,
}
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</filesystem>\n");
- return 0;
+
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&driverBuf);
+
+ return ret;
}


@@ -25847,13 +25860,14 @@ virDomainSoundDefFormat(virBufferPtr buf,
const char *model = virDomainSoundModelTypeToString(def->model);
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
size_t i;
+ int ret = -1;

virBufferSetChildIndent(&childBuf, buf);

if (!model) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected sound model %d"), def->model);
- return -1;
+ goto cleanup;
}

for (i = 0; i < def->ncodecs; i++)
@@ -25862,7 +25876,7 @@ virDomainSoundDefFormat(virBufferPtr buf,
virDomainDeviceInfoFormat(&childBuf, &def->info, flags);

if (virBufferCheckError(&childBuf) < 0)
- return -1;
+ goto cleanup;

virBufferAsprintf(buf, "<sound model='%s'", model);
if (virBufferUse(&childBuf)) {
@@ -25873,6 +25887,11 @@ virDomainSoundDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&childBuf);
+
return 0;
}

@@ -25884,11 +25903,12 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
{
const char *model = virDomainMemballoonModelTypeToString(def->model);
virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

if (!model) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected memballoon model %d"), def->model);
- return -1;
+ goto cleanup;
}

virBufferAsprintf(buf, "<memballoon model='%s'", model);
@@ -25909,10 +25929,9 @@ virDomainMemballoonDefFormat(virBufferPtr buf,

virDomainVirtioOptionsFormat(&driverBuf, def->virtio);

- if (virBufferCheckError(&driverBuf) < 0) {
- virBufferFreeAndReset(&childrenBuf);
- return -1;
- }
+ if (virBufferCheckError(&driverBuf) < 0)
+ goto cleanup;
+
if (virBufferUse(&driverBuf)) {
virBufferAddLit(&childrenBuf, "<driver");
virBufferAddBuffer(&childrenBuf, &driverBuf);
@@ -25921,7 +25940,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
}

if (virBufferCheckError(&childrenBuf) < 0)
- return -1;
+ goto cleanup;

if (!virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, "/>\n");
@@ -25931,6 +25950,11 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "</memballoon>\n");
}

+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&childrenBuf);
+
return 0;
}

@@ -25958,25 +25982,26 @@ virDomainWatchdogDefFormat(virBufferPtr buf,
const char *model = virDomainWatchdogModelTypeToString(def->model);
const char *action = virDomainWatchdogActionTypeToString(def->action);
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

virBufferSetChildIndent(&childBuf, buf);

if (!model) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected watchdog model %d"), def->model);
- return -1;
+ goto cleanup;
}

if (!action) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected watchdog action %d"), def->action);
- return -1;
+ goto cleanup;
}

virDomainDeviceInfoFormat(&childBuf, &def->info, flags);

if (virBufferCheckError(&childBuf) < 0)
- return -1;
+ goto cleanup;

virBufferAsprintf(buf, "<watchdog model='%s' action='%s'",
model, action);
@@ -25989,13 +26014,19 @@ virDomainWatchdogDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

- return 0;
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&childBuf);
+
+ return ret;
}

static int virDomainPanicDefFormat(virBufferPtr buf,
virDomainPanicDefPtr def)
{
virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

virBufferAddLit(buf, "<panic");

@@ -26007,7 +26038,7 @@ static int virDomainPanicDefFormat(virBufferPtr buf,
virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0);

if (virBufferCheckError(&childrenBuf) < 0)
- return -1;
+ goto cleanup;

if (virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, ">\n");
@@ -26016,8 +26047,13 @@ static int virDomainPanicDefFormat(virBufferPtr buf,
} else {
virBufferAddLit(buf, "/>\n");
}
+
+ ret = 0;
+
+ cleanup:
virBufferFreeAndReset(&childrenBuf);
- return 0;
+
+ return ret;
}

static int
@@ -26067,6 +26103,7 @@ virDomainRNGDefFormat(virBufferPtr buf,
const char *model = virDomainRNGModelTypeToString(def->model);
const char *backend = virDomainRNGBackendTypeToString(def->backend);
virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

virBufferAsprintf(buf, "<rng model='%s'>\n", model);
virBufferAdjustIndent(buf, 2);
@@ -26085,11 +26122,11 @@ virDomainRNGDefFormat(virBufferPtr buf,

case VIR_DOMAIN_RNG_BACKEND_EGD:
if (virDomainChrAttrsDefFormat(buf, def->source.chardev, false) < 0)
- return -1;
+ goto cleanup;
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
if (virDomainChrSourceDefFormat(buf, def->source.chardev, flags) < 0)
- return -1;
+ goto cleanup;
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</backend>\n");

@@ -26099,7 +26136,7 @@ virDomainRNGDefFormat(virBufferPtr buf,

virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
if (virBufferCheckError(&driverBuf) < 0)
- return -1;
+ goto cleanup;

if (virBufferUse(&driverBuf)) {
virBufferAddLit(buf, "<driver");
@@ -26111,7 +26148,13 @@ virDomainRNGDefFormat(virBufferPtr buf,

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</rng>\n");
- return 0;
+
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&driverBuf);
+
+ return ret;
}

void
@@ -26258,18 +26301,19 @@ virDomainVideoDefFormat(virBufferPtr buf,
{
const char *model = virDomainVideoTypeToString(def->type);
virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

if (!model) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected video model %d"), def->type);
- return -1;
+ goto cleanup;
}

virBufferAddLit(buf, "<video>\n");
virBufferAdjustIndent(buf, 2);
virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
if (virBufferCheckError(&driverBuf) < 0)
- return -1;
+ goto cleanup;
if (virBufferUse(&driverBuf) || (def->driver && def->driver->vgaconf)) {
virBufferAddLit(buf, "<driver");
if (virBufferUse(&driverBuf))
@@ -26308,7 +26352,13 @@ virDomainVideoDefFormat(virBufferPtr buf,

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</video>\n");
- return 0;
+
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&driverBuf);
+
+ return ret;
}

static int
@@ -26320,6 +26370,7 @@ virDomainInputDefFormat(virBufferPtr buf,
const char *bus = virDomainInputBusTypeToString(def->bus);
virBuffer childbuf = VIR_BUFFER_INITIALIZER;
virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

/* don't format keyboard into migratable XML for backward compatibility */
if (flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
@@ -26331,12 +26382,12 @@ virDomainInputDefFormat(virBufferPtr buf,
if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected input type %d"), def->type);
- return -1;
+ goto cleanup;
}
if (!bus) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected input bus type %d"), def->bus);
- return -1;
+ goto cleanup;
}

virBufferAsprintf(buf, "<input type='%s' bus='%s'",
@@ -26345,7 +26396,7 @@ virDomainInputDefFormat(virBufferPtr buf,
virBufferSetChildIndent(&childbuf, buf);
virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
if (virBufferCheckError(&driverBuf) < 0)
- return -1;
+ goto cleanup;

if (virBufferUse(&driverBuf)) {
virBufferAddLit(&childbuf, "<driver");
@@ -26356,7 +26407,7 @@ virDomainInputDefFormat(virBufferPtr buf,
virDomainDeviceInfoFormat(&childbuf, &def->info, flags);

if (virBufferCheckError(&childbuf) < 0)
- return -1;
+ goto cleanup;

if (!virBufferUse(&childbuf)) {
virBufferAddLit(buf, "/>\n");
@@ -26366,7 +26417,13 @@ virDomainInputDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "</input>\n");
}

- return 0;
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&childbuf);
+ virBufferFreeAndReset(&driverBuf);
+
+ return ret;
}


@@ -27028,19 +27085,20 @@ virDomainHubDefFormat(virBufferPtr buf,
{
const char *type = virDomainHubTypeToString(def->type);
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

virBufferSetChildIndent(&childBuf, buf);

if (!type) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected hub type %d"), def->type);
- return -1;
+ goto cleanup;
}

virDomainDeviceInfoFormat(&childBuf, &def->info, flags);

if (virBufferCheckError(&childBuf) < 0)
- return -1;
+ goto cleanup;

virBufferAsprintf(buf, "<hub type='%s'", type);
if (virBufferUse(&childBuf)) {
@@ -27051,7 +27109,12 @@ virDomainHubDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

- return 0;
+ ret = 0;
+
+ cleanup:
+ virBufferFreeAndReset(&childBuf);
+
+ return ret;
}

[Pavel Hrdina]

Post by Andrea Bolognani
In many cases, an early exit from a function would cause
memory allocated by local virBuffer instances not to be
released.
Provide proper cleanup paths to solve the issue.
---
src/conf/domain_conf.c | 137 ++++++++++++++++++++++++++++++-----------
1 file changed, 100 insertions(+), 37 deletions(-)

Reviewed-by: Pavel Hrdina <***@redhat.com>

[Radoslaw Biernacki]

This patch breaks the error return value for:
virDomainControllerDefFormat()
virDomainSoundDefFormat()
virDomainMemballoonDefFormat()

Patch adds the "ret" variable but in error exit it use "return 0" statement.
Actually this breaks compilation.
Was this code compiled ?

conf/domain_conf.c: In function 'virDomainControllerDefFormat':
conf/domain_conf.c:24368:9: error: variable 'ret' set but not used
[-Werror=unused-but-set-variable]
int ret = -1;
^~~
CC test/libvirt_driver_test_la-test_driver.lo
CC vmx/libvirt_vmx_la-vmx.lo
CC vmware/libvirt_driver_vmware_la-vmware_driver.lo
CC vmware/libvirt_driver_vmware_la-vmware_conf.lo
conf/domain_conf.c: In function 'virDomainSoundDefFormat':
conf/domain_conf.c:25882:9: error: variable 'ret' set but not used
[-Werror=unused-but-set-variable]
int ret = -1;
^~~
conf/domain_conf.c: In function 'virDomainMemballoonDefFormat':
conf/domain_conf.c:25926:9: error: variable 'ret' set but not used
[-Werror=unused-but-set-variable]
int ret = -1;
^~~

Post by Andrea Bolognani
In many cases, an early exit from a function would cause
memory allocated by local virBuffer instances not to be
released.
Provide proper cleanup paths to solve the issue.
---
src/conf/domain_conf.c | 137 ++++++++++++++++++++++++++++++-----------
1 file changed, 100 insertions(+), 37 deletions(-)

--
libvir-list mailing list
https://www.redhat.com/mailman/listinfo/libvir-list

[Andrea Bolognani]

virXMLFormatElement() might fail, but we were not checking
its return value.

Fixing this requires us to change virDomainDeviceInfoFormat()
so that it can report an error back to the caller.

Introduced-by: 0d6b87335c00451b0923ecc91d617f71e4135bf8
Spotted-by: Coverity
Reported-by: John Ferlan <***@redhat.com>
Signed-off-by: Andrea Bolognani <***@redhat.com>
---
src/conf/domain_conf.c | 97 ++++++++++++++++++++++++++++--------------
1 file changed, 66 insertions(+), 31 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1d04cac104..8cb9b2719c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6418,13 +6418,14 @@ virDomainVirtioOptionsFormat(virBufferPtr buf,
}


-static void ATTRIBUTE_NONNULL(2)
+static int ATTRIBUTE_NONNULL(2)
virDomainDeviceInfoFormat(virBufferPtr buf,
virDomainDeviceInfoPtr info,
unsigned int flags)
{
virBuffer attrBuf = VIR_BUFFER_INITIALIZER;
virBuffer childBuf = VIR_BUFFER_INITIALIZER;
+ int ret = -1;

if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex);
@@ -6467,8 +6468,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
}

if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
- info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)
- return;
+ info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390) {
+ /* We're done here */
+ ret = 0;
+ goto cleanup;
+ }

virBufferAsprintf(&attrBuf, " type='%s'",
virDomainDeviceAddressTypeToString(info->type));
@@ -6562,10 +6566,16 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
break;
}

- virXMLFormatElement(buf, "address", &attrBuf, &childBuf);
+ if (virXMLFormatElement(buf, "address", &attrBuf, &childBuf) < 0)
+ goto cleanup;
+
+ ret = 0;

+ cleanup:
virBufferFreeAndReset(&attrBuf);
virBufferFreeAndReset(&childBuf);
+
+ return ret;
}

static int
@@ -24299,8 +24309,10 @@ virDomainDiskDefFormat(virBufferPtr buf,
if (def->src->encryption && !def->src->encryptionInherited &&
virStorageEncryptionFormat(buf, def->src->encryption) < 0)
return -1;
- virDomainDeviceInfoFormat(buf, &def->info,
- flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT);
+ if (virDomainDeviceInfoFormat(buf, &def->info,
+ flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) {
+ return -1;
+ }

if (virDomainDiskDefFormatPrivateData(buf, def, flags, xmlopt) < 0)
return -1;
@@ -24475,7 +24487,8 @@ virDomainControllerDefFormat(virBufferPtr buf,

virDomainControllerDriverFormat(&childBuf, def);

- virDomainDeviceInfoFormat(&childBuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0)
+ goto cleanup;

if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
def->opts.pciopts.pcihole64) {
@@ -24613,7 +24626,8 @@ virDomainFSDefFormat(virBufferPtr buf,
if (def->readonly)
virBufferAddLit(buf, "<readonly/>\n");

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ goto cleanup;

if (def->space_hard_limit)
virBufferAsprintf(buf, "<space_hard_limit unit='bytes'>"
@@ -25412,9 +25426,11 @@ virDomainNetDefFormat(virBufferPtr buf,

virDomainNetDefCoalesceFormatXML(buf, def->coalesce);

- virDomainDeviceInfoFormat(buf, &def->info,
- flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT
- | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM);
+ if (virDomainDeviceInfoFormat(buf, &def->info,
+ flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT
+ | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) {
+ return -1;
+ }

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</interface>\n");
@@ -25723,7 +25739,8 @@ virDomainChrDefFormat(virBufferPtr buf,
if (virDomainChrTargetDefFormat(buf, def, flags) < 0)
return -1;

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ return -1;

virBufferAdjustIndent(buf, -2);
virBufferAsprintf(buf, "</%s>\n", elementName);
@@ -25772,7 +25789,8 @@ virDomainSmartcardDefFormat(virBufferPtr buf,
_("unexpected smartcard type %d"), def->type);
goto cleanup;
}
- virDomainDeviceInfoFormat(&childBuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0)
+ goto cleanup;

if (virBufferCheckError(&childBuf) < 0)
goto cleanup;
@@ -25843,7 +25861,8 @@ virDomainTPMDefFormat(virBufferPtr buf,
break;
}

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ return -1;

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</tpm>\n");
@@ -25873,7 +25892,8 @@ virDomainSoundDefFormat(virBufferPtr buf,
for (i = 0; i < def->ncodecs; i++)
virDomainSoundCodecDefFormat(&childBuf, def->codecs[i]);

- virDomainDeviceInfoFormat(&childBuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0)
+ goto cleanup;

if (virBufferCheckError(&childBuf) < 0)
goto cleanup;
@@ -25922,7 +25942,8 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
if (def->period)
virBufferAsprintf(&childrenBuf, "<stats period='%i'/>\n", def->period);

- virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0)
+ goto cleanup;

if (def->virtio) {
virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
@@ -25965,7 +25986,8 @@ virDomainNVRAMDefFormat(virBufferPtr buf,
{
virBufferAddLit(buf, "<nvram>\n");
virBufferAdjustIndent(buf, 2);
- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ return -1;

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</nvram>\n");
@@ -25998,7 +26020,8 @@ virDomainWatchdogDefFormat(virBufferPtr buf,
goto cleanup;
}

- virDomainDeviceInfoFormat(&childBuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0)
+ goto cleanup;

if (virBufferCheckError(&childBuf) < 0)
goto cleanup;
@@ -26035,7 +26058,8 @@ static int virDomainPanicDefFormat(virBufferPtr buf,
virDomainPanicModelTypeToString(def->model));

virBufferSetChildIndent(&childrenBuf, buf);
- virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0);
+ if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0)
+ goto cleanup;

if (virBufferCheckError(&childrenBuf) < 0)
goto cleanup;
@@ -26087,7 +26111,8 @@ virDomainShmemDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ return -1;

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</shmem>\n");
@@ -26144,7 +26169,8 @@ virDomainRNGDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ goto cleanup;

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</rng>\n");
@@ -26271,7 +26297,8 @@ virDomainMemoryDefFormat(virBufferPtr buf,

virDomainMemoryTargetDefFormat(buf, def);

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ return -1;

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</memory>\n");
@@ -26348,7 +26375,8 @@ virDomainVideoDefFormat(virBufferPtr buf,
virBufferAddLit(buf, "/>\n");
}

- virDomainDeviceInfoFormat(buf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
+ goto cleanup;

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</video>\n");
@@ -26404,7 +26432,8 @@ virDomainInputDefFormat(virBufferPtr buf,
virBufferAddLit(&childbuf, "/>\n");
}
virBufferEscapeString(&childbuf, "<source evdev='%s'/>\n", def->source.evdev);
- virDomainDeviceInfoFormat(&childbuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childbuf, &def->info, flags) < 0)
+ goto cleanup;

if (virBufferCheckError(&childbuf) < 0)
goto cleanup;
@@ -27004,9 +27033,11 @@ virDomainHostdevDefFormat(virBufferPtr buf,
if (def->shareable)
virBufferAddLit(buf, "<shareable/>\n");

- virDomainDeviceInfoFormat(buf, def->info,
- flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT
- | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM);
+ if (virDomainDeviceInfoFormat(buf, def->info,
+ flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT
+ | VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM) < 0) {
+ return -1;
+ }

virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</hostdev>\n");
@@ -27032,8 +27063,10 @@ virDomainRedirdevDefFormat(virBufferPtr buf,
if (virDomainChrSourceDefFormat(buf, def->source, flags) < 0)
return -1;

- virDomainDeviceInfoFormat(buf, &def->info,
- flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT);
+ if (virDomainDeviceInfoFormat(buf, &def->info,
+ flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) < 0) {
+ return -1;
+ }
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</redirdev>\n");
return 0;
@@ -27095,7 +27128,8 @@ virDomainHubDefFormat(virBufferPtr buf,
goto cleanup;
}

- virDomainDeviceInfoFormat(&childBuf, &def->info, flags);
+ if (virDomainDeviceInfoFormat(&childBuf, &def->info, flags) < 0)
+ goto cleanup;

if (virBufferCheckError(&childBuf) < 0)
goto cleanup;
@@ -27816,7 +27850,8 @@ virDomainVsockDefFormat(virBufferPtr buf,
if (virXMLFormatElement(&childBuf, "cid", &cidAttrBuf, NULL) < 0)
goto cleanup;

- virDomainDeviceInfoFormat(&childBuf, &vsock->info, 0);
+ if (virDomainDeviceInfoFormat(&childBuf, &vsock->info, 0) < 0)
+ goto cleanup;

if (virXMLFormatElement(buf, "vsock", &attrBuf, &childBuf) < 0)
goto cleanup;

[Pavel Hrdina]

Post by Andrea Bolognani
virXMLFormatElement() might fail, but we were not checking
its return value.
Fixing this requires us to change virDomainDeviceInfoFormat()
so that it can report an error back to the caller.
Introduced-by: 0d6b87335c00451b0923ecc91d617f71e4135bf8
Spotted-by: Coverity
---
src/conf/domain_conf.c | 97 ++++++++++++++++++++++++++++--------------
1 file changed, 66 insertions(+), 31 deletions(-)

Reviewed-by: Pavel Hrdina <***@redhat.com>

[Pavel Hrdina]

Post by Andrea Bolognani
This avoids setting 'ret' multiple times, which will result
in errors being masked if the first operation fails but the
second one succeeds.
Introduced-by: f183b87fc1dbcc6446ac3c1cef9cdd345b9725fb
Spotted-by: Coverity
---
src/conf/domain_addr.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Pavel Hrdina <***@redhat.com>