[libvirt] [PATCH] fix errors in virReportSystemErrorFull

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> wrote:
...
> Looking at the whole method again, I think it needs to be re-written to
> something closer to this:

Ok, I've adapted that.
Changes:
  - handle snprintf and vsnprintf failure
  - insert ": " into the result string
  - use stpcpy, not strcat (as a general waste-avoidance measure,
      -- i.e., don't traverse potentially long strings unnecessarily --
      no linux- or gnulib-using project should use strcat on
      arbitrary strings.)

That latter one required pulling in the stpcpy module from gnulib.
Linux has had stpcpy for ages, and POSIX now specifies it,
but Solaris 10 and others still lack it.
So there are two more patches that are prerequisites for this one:
  - update from gnulib
  - add stpcpy to bootstrap's module list and pull in the
      required gnulib files (2 new, 3 changed)

I'm checking all of this in shortly.

>From 5c7812209b93654a0b236ef9529fdebe49af3972 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Mon, 26 Jan 2009 14:59:35 +0100
Subject: [PATCH] fix errors in virReportSystemErrorFull

* src/virterror.c (virStrerror): New function.
(virReportSystemErrorFull): Don't leak "combined".
In fact, don't even attempt allocation.
Do include the result of formatted print in final diagnostic.
---
 src/virterror.c |   72 +++++++++++++++++++++++++++++++-----------------------
 1 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/src/virterror.c b/src/virterror.c
index 0c66781..a577002 100644
--- a/src/virterror.c
+++ b/src/virterror.c
@@ -1005,57 +1005,67 @@ void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode,

 }

-
-void virReportSystemErrorFull(virConnectPtr conn,
-                              int domcode,
-                              int theerrno,
-                              const char *filename ATTRIBUTE_UNUSED,
-                              const char *funcname ATTRIBUTE_UNUSED,
-                              size_t linenr ATTRIBUTE_UNUSED,
-                              const char *fmt, ...)
+static const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen)
 {
-    va_list args;
-    char errorMessage[1024];
-    char systemError[1024];
-    char *theerrnostr;
-    const char *virerr;
-    char *combined = NULL;
-
 #ifdef HAVE_STRERROR_R
-#ifdef __USE_GNU
+# ifdef __USE_GNU
     /* Annoying linux specific API contract */
-    theerrnostr = strerror_r(theerrno, systemError, sizeof(systemError));
-#else
-    strerror_r(theerrno, systemError, sizeof(systemError));
-    theerrnostr = systemError;
-#endif
+    return strerror_r(theerrno, errBuf, errBufLen);
+# else
+    strerror_r(theerrno, errBuf, errBufLen);
+    return errBuf;
+# endif
 #else
     /* Mingw lacks strerror_r() and its strerror() is definitely not
      * threadsafe, so safest option is to just print the raw errno
      * value - we can at least reliably & safely look it up in the
      * header files for debug purposes
      */
-    snprintf(systemError, sizeof(systemError), "errno=%d", theerrno);
-    theerrnostr = systemError;
+    int n = snprintf(errBuf, errBufLen, "errno=%d", theerrno);
+    return (0 < n && n < errBufLen
+            ? errBuf : _("internal error: buffer too small"));
 #endif
+}
+
+void virReportSystemErrorFull(virConnectPtr conn,
+                              int domcode,
+                              int theerrno,
+                              const char *filename ATTRIBUTE_UNUSED,
+                              const char *funcname ATTRIBUTE_UNUSED,
+                              size_t linenr ATTRIBUTE_UNUSED,
+                              const char *fmt, ...)
+{
+    char strerror_buf[1024];
+    char msgDetailBuf[1024];
+
+    const char *errnoDetail = virStrerror(theerrno, strerror_buf,
+                                          sizeof(strerror_buf));
+    const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt);
+    const char *msgDetail = NULL;

     if (fmt) {
+        va_list args;
+        int n;
+
         va_start(args, fmt);
-        vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args);
+        n = vsnprintf(msgDetailBuf, sizeof(msgDetailBuf), fmt, args);
         va_end(args);
-    } else {
-        errorMessage[0] = '\0';
+
+        size_t len = strlen (msgDetailBuf);
+        if (0 <= n && n + 2 + len < sizeof (msgDetailBuf)) {
+          char *p = msgDetailBuf + n;
+          stpcpy (stpcpy (p, ": "), errnoDetail);
+          msgDetail = msgDetailBuf;
+        }
     }

-    if (virAsprintf(&combined, "%s: %s", errorMessage, theerrnostr) < 0)
-        combined = theerrnostr; /* OOM, so lets just pass the strerror info as best effort */
+    if (!msgDetailBuf)
+        msgDetail = errnoDetail;

-    virerr = virErrorMsg(VIR_ERR_SYSTEM_ERROR, (errorMessage[0] ? errorMessage : NULL));
     virRaiseError(conn, NULL, NULL, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR,
-                  virerr, errorMessage, NULL, -1, -1, virerr, errorMessage);
+                  msg, msgDetail, NULL, -1, -1, msg, msgDetail);
 }

-
 void virReportOOMErrorFull(virConnectPtr conn,
                            int domcode,
                            const char *filename ATTRIBUTE_UNUSED,
--
1.6.1.1.363.g2a3bd