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@xxxxxxxxxx>
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,
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
