This was (probably) a relict from times when we cared about OOM conditions and the possibility to report the error. Nowadays it doesn't make sense as virRaiseErrorFull will do an allocated copy of the strings and also concatenate the error message prefix with the detail which doesn't guarantee that the result will be less than 1024 chars. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virerror.c | 47 +++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index a503cdefdc..b9a49ec70c 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -28,6 +28,7 @@ #include "virlog.h" #include "virthread.h" #include "virstring.h" +#include "virbuffer.h" #define LIBVIRT_VIRERRORPRIV_H_ALLOW #include "virerrorpriv.h" @@ -1285,23 +1286,23 @@ void virReportErrorHelper(int domcode, const char *fmt, ...) { int save_errno = errno; - va_list args; - char errorMessage[VIR_ERROR_MAX_LENGTH]; - const char *virerr; + g_autofree char *detail = NULL; + const char *errormsg; if (fmt) { + va_list args; + va_start(args, fmt); - g_vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); + detail = g_strdup_vprintf(fmt, args); va_end(args); - } else { - errorMessage[0] = '\0'; } - virerr = virErrorMsg(errorcode, (errorMessage[0] ? errorMessage : NULL)); + errormsg = virErrorMsg(errorcode, detail); + virRaiseErrorFull(filename, funcname, linenr, domcode, errorcode, VIR_ERR_ERROR, - virerr, errorMessage, NULL, - -1, -1, virerr, errorMessage); + errormsg, detail, NULL, + -1, -1, errormsg, detail); errno = save_errno; } @@ -1325,36 +1326,28 @@ void virReportSystemErrorFull(int domcode, const char *fmt, ...) { int save_errno = errno; - char msgDetailBuf[VIR_ERROR_MAX_LENGTH]; - - const char *errnoDetail = g_strerror(theerrno); - const char *msg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, fmt); - const char *msgDetail = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + g_autofree char *detail = NULL; + const char *errormsg; if (fmt) { va_list args; - size_t len; - int n; va_start(args, fmt); - n = g_vsnprintf(msgDetailBuf, sizeof(msgDetailBuf), fmt, args); + virBufferVasprintf(&buf, fmt, args); va_end(args); - len = strlen(errnoDetail); - if (0 <= n && n + 2 + len < sizeof(msgDetailBuf)) { - strcpy(msgDetailBuf + n, ": "); - n += 2; - strcpy(msgDetailBuf + n, errnoDetail); - msgDetail = msgDetailBuf; - } + virBufferAddLit(&buf, ": "); } - if (!msgDetail) - msgDetail = errnoDetail; + virBufferAdd(&buf, g_strerror(theerrno), -1); + + detail = virBufferContentAndReset(&buf); + errormsg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, detail); virRaiseErrorFull(filename, funcname, linenr, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, - msg, msgDetail, NULL, theerrno, -1, msg, msgDetail); + errormsg, detail, NULL, theerrno, -1, errormsg, detail); errno = save_errno; } -- 2.29.2

Some error message reporting functions already have allocated buffers which were used to format the error message, so copying the strings is redundant. Extract the internals from 'virRaiseErrorFull' to 'virRaiseErrorInternal' which takes allocated strings as arguments and steals them, so that callers can reuse the buffers. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virerror.c | 159 ++++++++++++++++++++++++++------------------ 1 file changed, 95 insertions(+), 64 deletions(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index b9a49ec70c..220fc362c1 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -755,6 +755,70 @@ void virRaiseErrorLog(const char *filename, meta, "%s", err->message); } + +/** + * virRaiseErrorInternal: + * + * Internal helper to assign and raise error. Note that @msgarg, @str1arg, + * @str2arg and @str3arg if non-NULL must be heap-allocated strings and are + * stolen and freed by this function. + */ +static void +virRaiseErrorInternal(const char *filename, + const char *funcname, + size_t linenr, + int domain, + int code, + virErrorLevel level, + char *msgarg, + char *str1arg, + char *str2arg, + char *str3arg, + int int1, + int int2) +{ + g_autofree char *msg = msgarg; + g_autofree char *str1 = str1arg; + g_autofree char *str2 = str2arg; + g_autofree char *str3 = str3arg; + virErrorPtr to; + virLogMetadata meta[] = { + { .key = "LIBVIRT_DOMAIN", .s = NULL, .iv = domain }, + { .key = "LIBVIRT_CODE", .s = NULL, .iv = code }, + { .key = NULL }, + }; + + /* + * All errors are recorded in thread local storage + * For compatibility, public API calls will copy them + * to the per-connection error object when necessary + */ + if (!(to = virLastErrorObject())) + return; + + virResetError(to); + + if (code == VIR_ERR_OK) + return; + + if (!msg) + msg = g_strdup(_("No error message provided")); + + /* Deliberately not setting conn, dom & net fields sincethey're utterly unsafe. */ + to->domain = domain; + to->code = code; + to->message = g_steal_pointer(&msg); + to->level = level; + to->str1 = g_steal_pointer(&str1); + to->str2 = g_steal_pointer(&str2); + to->str3 = g_steal_pointer(&str3); + to->int1 = int1; + to->int2 = int2; + + virRaiseErrorLog(filename, funcname, linenr, to, meta); +} + + /** * virRaiseErrorFull: * @filename: filename where error was raised @@ -789,63 +853,20 @@ virRaiseErrorFull(const char *filename, const char *fmt, ...) { int save_errno = errno; - virErrorPtr to; - char *str; - virLogMetadata meta[] = { - { .key = "LIBVIRT_DOMAIN", .s = NULL, .iv = domain }, - { .key = "LIBVIRT_CODE", .s = NULL, .iv = code }, - { .key = NULL }, - }; - - /* - * All errors are recorded in thread local storage - * For compatibility, public API calls will copy them - * to the per-connection error object when necessary - */ - to = virLastErrorObject(); - if (!to) { - errno = save_errno; - return; /* Hit OOM allocating thread error object, sod all we can do now */ - } - - virResetError(to); + char *msg = NULL; - if (code == VIR_ERR_OK) { - errno = save_errno; - return; - } - - /* - * formats the message; drop message on OOM situations - */ - if (fmt == NULL) { - str = g_strdup(_("No error message provided")); - } else { + if (fmt) { va_list ap; + va_start(ap, fmt); - str = g_strdup_vprintf(fmt, ap); + msg = g_strdup_vprintf(fmt, ap); va_end(ap); } - /* - * Save the information about the error - */ - /* - * Deliberately not setting conn, dom & net fields since - * they're utterly unsafe - */ - to->domain = domain; - to->code = code; - to->message = str; - to->level = level; - to->str1 = g_strdup(str1); - to->str2 = g_strdup(str2); - to->str3 = g_strdup(str3); - to->int1 = int1; - to->int2 = int2; - - virRaiseErrorLog(filename, funcname, linenr, - to, meta); + virRaiseErrorInternal(filename, funcname, linenr, + domain, code, level, + msg, g_strdup(str1), g_strdup(str2), g_strdup(str3), + int1, int2); errno = save_errno; } @@ -1286,8 +1307,9 @@ void virReportErrorHelper(int domcode, const char *fmt, ...) { int save_errno = errno; - g_autofree char *detail = NULL; - const char *errormsg; + char *detail = NULL; + char *errormsg = NULL; + char *fullmsg = NULL; if (fmt) { va_list args; @@ -1297,12 +1319,19 @@ void virReportErrorHelper(int domcode, va_end(args); } - errormsg = virErrorMsg(errorcode, detail); + errormsg = g_strdup(virErrorMsg(errorcode, detail)); + + if (errormsg) { + if (detail) + fullmsg = g_strdup_printf(errormsg, detail); + else + fullmsg = g_strdup(errormsg); + } + + virRaiseErrorInternal(filename, funcname, linenr, + domcode, errorcode, VIR_ERR_ERROR, + fullmsg, errormsg, detail, NULL, -1, -1); - virRaiseErrorFull(filename, funcname, linenr, - domcode, errorcode, VIR_ERR_ERROR, - errormsg, detail, NULL, - -1, -1, errormsg, detail); errno = save_errno; } @@ -1327,8 +1356,9 @@ void virReportSystemErrorFull(int domcode, { int save_errno = errno; virBuffer buf = VIR_BUFFER_INITIALIZER; - g_autofree char *detail = NULL; - const char *errormsg; + char *detail = NULL; + char *errormsg = NULL; + char *fullmsg = NULL; if (fmt) { va_list args; @@ -1343,11 +1373,12 @@ void virReportSystemErrorFull(int domcode, virBufferAdd(&buf, g_strerror(theerrno), -1); detail = virBufferContentAndReset(&buf); - errormsg = virErrorMsg(VIR_ERR_SYSTEM_ERROR, detail); + errormsg = g_strdup(virErrorMsg(VIR_ERR_SYSTEM_ERROR, detail)); + fullmsg = g_strdup_printf(errormsg, detail); - virRaiseErrorFull(filename, funcname, linenr, - domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, - errormsg, detail, NULL, theerrno, -1, errormsg, detail); + virRaiseErrorInternal(filename, funcname, linenr, + domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, + fullmsg, errormsg, detail, NULL, theerrno, -1); errno = save_errno; } -- 2.29.2

Now that error message formatting doesn't use fixed size buffers we can drop the math for calculating the maximum chunk of log to report in the error message and use a round number. This also makes it obvious that the chosen number is arbitrary. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_process.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 398f63282e..55649286ae 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2140,14 +2140,9 @@ qemuProcessReportLogError(qemuDomainLogContextPtr logCtxt, const char *msgprefix) { g_autofree char *logmsg = NULL; - size_t max; - max = VIR_ERROR_MAX_LENGTH - 1; - max -= strlen(msgprefix); - /* The length of the formatting string minus two '%s' */ - max -= strlen(_("%s: %s")) - 4; - - if (qemuProcessReadLog(logCtxt, &logmsg, max) < 0) + /* assume that 1024 chars of qemu log is the right balance */ + if (qemuProcessReadLog(logCtxt, &logmsg, 1024) < 0) return -1; virResetLastError(); -- 2.29.2

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virerror.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virerror.h b/src/util/virerror.h index da7d7c0afe..bec3863c3f 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -23,8 +23,6 @@ #include "internal.h" -#define VIR_ERROR_MAX_LENGTH 1024 - extern virErrorFunc virErrorHandler; extern void *virUserData; -- 2.29.2