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@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,
[...]