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