[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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,

[...]


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]