Re: [libvirt] [PATCH 3/5] virstring: Introduce virVasprintfNOOM and make virVasprintf report OOM
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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)