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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list