[libvirt] [PATCH 3/5] virstring: Introduce virVasprintfNOOM and make virVasprintf report OOM

[Eric Blake]

On 04/02/2013 08:22 AM, Michal Privoznik wrote:
> ---
>  src/libvirt_private.syms |  1 +
>  src/qemu/qemu_domain.c   |  4 +---
>  src/util/viraudit.c      |  2 +-
>  src/util/vircommand.c    |  4 ++--
>  src/util/virerror.c      |  2 +-
>  src/util/virlog.c        |  2 +-
>  src/util/virstring.c     | 22 ++++++++++++++++++++--
>  src/util/virstring.h     |  3 +++
>  8 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5060b87..8d1abe7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1742,6 +1742,7 @@ virStrToLong_ul;
>  virStrToLong_ull;
>  virTrimSpaces;
>  virVasprintf;
> +virVasprintfNOOM;

Bike-shedding: Maybe virVasprintfQuiet works better?  I'm a bit worried
that the NOOM suffix will cause more questions than it resolves about
what it is really doing, which is being silent on OOM conditions.

> +++ b/src/qemu/qemu_domain.c
> @@ -1477,10 +1477,8 @@ int qemuDomainAppendLog(virQEMUDriverPtr driver,
>          (fd = qemuDomainCreateLog(driver, obj, true)) < 0)
>          goto cleanup;
>  
> -    if (virVasprintf(&message, fmt, argptr) < 0) {
> -        virReportOOMError();
> +    if (virVasprintf(&message, fmt, argptr) < 0)
>          goto cleanup;
> -    }

This hunk looks like it is in the wrong patch.  Probably meant for 5/5?

> +++ b/src/util/viraudit.c
> @@ -96,7 +96,7 @@ void virAuditSend(const char *filename,
>  #endif
>  
>      va_start(args, fmt);
> -    if (virVasprintf(&str, fmt, args) < 0) {
> +    if (virVasprintfNOOM(&str, fmt, args) < 0) {
>          VIR_WARN("Out of memory while formatting audit message");
>          str = NULL;

The str=NULL assignment is redundant with the guarantees of
virVasprintf*(), if you want to clean that up while you are touching
this area of code.

> +++ b/src/util/virerror.c
> @@ -649,7 +649,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
>      } else {
>          va_list ap;
>          va_start(ap, fmt);
> -        ignore_value(virVasprintf(&str, fmt, ap));
> +        ignore_value(virVasprintfNOOM(&str, fmt, ap));

Given how seldom virVasprintfNOOM (or whatever name we give it) will be
used, it probably doesn't need ATTRIBUTE_RETURN_CHECK.  After all, if
you know you don't care about an error being raised, then the compiler
shouldn't make you care about a successful return value either.

> @@ -327,6 +327,23 @@ virVasprintf(char **strp, const char *fmt, va_list list)
>  }
>  
>  /**
> + * virVasprintf
> + *
> + * like glibc's vasprintf but makes sure *strp == NULL on failure and reports
> + * OOM error.
> + */
> +int
> +virVasprintf(char **strp, const char *fmt, va_list list)
> +{
> +    int ret;
> +
> +    if ((ret = virVasprintfNOOM(strp, fmt, list)) == -1)

s/== -1/< 0/

The idea makes sense, and the choices that you converted over to using
the silent variant look correct, but a v2 might be useful, particularly
after my comments on 2/5 about splitting the series differently.  Anyone
else with a better idea for a name?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130402/dc66219c/attachment-0001.sig>