Re: [libvirt] [PATCH 7/8] conf: check for buffer errors before virBufferUse
On 07/26/2017 09:29 AM, Ján Tomko wrote:
After an OOM error, virBuffer* APIs set buf->use to zero.
Adding a buffer to the parent buffer only if use is non-zero
would quitely drop data on error.
s/quitely/quietly/
Check the error beforehand to make sure buf->use is zero
because we have not attempted to add anything to it.
---
src/conf/capabilities.c | 5 +++++
src/conf/cpu_conf.c | 4 ++++
src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++--
3 files changed, 38 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
One nit (feel free to ignore) and one concern follows...
John
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
index 0f99f3096..db7efffdf 100644
--- a/src/conf/capabilities.c
+++ b/src/conf/capabilities.c
@@ -930,6 +930,11 @@ virCapabilitiesFormatCaches(virBufferPtr buf,
bank->controls[j]->max_allocation);
}
One could move the VIR_FREE(cpus_str) to right here and avoid the two
It also seems this particular function uses virBufferAdjustIndent on buf
instead of on the child buf (or in this case controlBuf)...
+ if (virBufferCheckError(&controlBuf) < 0) {
+ VIR_FREE(cpus_str);
+ return -1;
+ }
+
if (virBufferUse(&controlBuf)) {
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &controlBuf);
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7728321cb..4dc49fdb0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
[...]
@@ -23309,6 +23321,10 @@ static int
virDomainPanicDefFormat(virBufferPtr buf,
virBufferAdjustIndent(&childrenBuf, indent + 2);
if (virDomainDeviceInfoFormat(&childrenBuf, &def->info, 0) < 0)
return -1;
+
+ if (virBufferCheckError(&childrenBuf) < 0)
+ return -1;
+
Seeing a virBufferFreeAndReset below this if condition first had me
thinking, well that's unnecessary; however, in actuality I think we have
a leak any time virBufferUse doesn't return a non zero value call
virBufferAddBuffer to consume the buffer.
Not necessarily something to stop this patch, but perhaps a leak for:
virDomainDiskDefFormat
virDomainControllerDriverFormat
virDomainControllerDefFormat
virDomainFSDefFormat
virDomainMemballoonDefFormat
virDomainRNGDefFormat
virDomainVideoDefFormat
virDomainInputDefFormat
virDomainIOMMUDefFormat
virDomainDiskDefFormat
if (virBufferUse(&childrenBuf)) {
virBufferAddLit(buf, ">\n");
virBufferAddBuffer(buf, &childrenBuf);
@@ -23655,6 +23671,9 @@ virDomainInputDefFormat(virBufferPtr buf,
[...]