Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2015-09-10' into staging

error: On abort, report where the error was created

# gpg: Signature made Thu 10 Sep 2015 13:01:39 BST using RSA key ID EB918653
# gpg: Good signature from "Markus Armbruster <armbru@redhat.com>"
# gpg:                 aka "Markus Armbruster <armbru@pond.sub.org>"

* remotes/armbru/tags/pull-error-2015-09-10:
  error: On abort, report where the error was created
  error: Revamp interface documentation
  error: error_set_errno() is unused, drop
  qga/vss-win32: Document the DLL requires non-null errp
  qga: Clean up unnecessarily dirty casts
  error: Make error_setg() a function
  error: De-duplicate code creating Error objects
Signed-off-by: NPeter Maydell <peter.maydell@linaro.org>

include/qapi/error.h

@@ -2,13 +2,75 @@
  * QEMU Error Objects
  *
  * Copyright IBM, Corp. 2011
+ * Copyright (C) 2011-2015 Red Hat, Inc.
  *
  * Authors:
  *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Markus Armbruster <armbru@redhat.com>
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.  See
  * the COPYING.LIB file in the top-level directory.
  */
+
+/*
+ * Error reporting system loosely patterned after Glib's GError.
+ *
+ * Create an error:
+ *     error_setg(&err, "situation normal, all fouled up");
+ *
+ * Report an error to stderr:
+ *     error_report_err(err);
+ * This frees the error object.
+ *
+ * Report an error somewhere else:
+ *     const char *msg = error_get_pretty(err);
+ *     do with msg what needs to be done...
+ *     error_free(err);
+ *
+ * Handle an error without reporting it (just for completeness):
+ *     error_free(err);
+ *
+ * Pass an existing error to the caller:
+ *     error_propagate(errp, err);
+ * where Error **errp is a parameter, by convention the last one.
+ *
+ * Create a new error and pass it to the caller:
+ *     error_setg(errp, "situation normal, all fouled up");
+ *
+ * Call a function and receive an error from it:
+ *     Error *err = NULL;
+ *     foo(arg, &err);
+ *     if (err) {
+ *         handle the error...
+ *     }
+ *
+ * Call a function ignoring errors:
+ *     foo(arg, NULL);
+ *
+ * Call a function aborting on errors:
+ *     foo(arg, &error_abort);
+ *
+ * Receive an error and pass it on to the caller:
+ *     Error *err = NULL;
+ *     foo(arg, &err);
+ *     if (err) {
+ *         handle the error...
+ *         error_propagate(errp, err);
+ *     }
+ * where Error **errp is a parameter, by convention the last one.
+ *
+ * Do *not* "optimize" this to
+ *     foo(arg, errp);
+ *     if (*errp) { // WRONG!
+ *         handle the error...
+ *     }
+ * because errp may be NULL!
+ *
+ * But when all you do with the error is pass it on, please use
+ *     foo(arg, errp);
+ * for readability.
+ */
+
 #ifndef ERROR_H
 #define ERROR_H
 
@@ -16,93 +78,125 @@
 #include "qapi-types.h"
 #include <stdbool.h>
 
-/**
- * A class representing internal errors within QEMU.  An error has a ErrorClass
- * code and a human message.
+/*
+ * Opaque error object.
  */
 typedef struct Error Error;
 
-/**
- * Set an indirect pointer to an error given a ErrorClass value and a
- * printf-style human message.  This function is not meant to be used outside
- * of QEMU.
+/*
+ * Get @err's human-readable error message.
  */
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
-    GCC_FMT_ATTR(3, 4);
+const char *error_get_pretty(Error *err);
 
-/**
- * Set an indirect pointer to an error given a ErrorClass value and a
- * printf-style human message, followed by a strerror() string if
- * @os_error is not zero.
+/*
+ * Get @err's error class.
+ * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
+ * strongly discouraged.
  */
-void error_set_errno(Error **errp, int os_error, ErrorClass err_class,
-                     const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+ErrorClass error_get_class(const Error *err);
 
-#ifdef _WIN32
-/**
- * Set an indirect pointer to an error given a ErrorClass value and a
- * printf-style human message, followed by a g_win32_error_message() string if
- * @win32_err is not zero.
+/*
+ * Create a new error object and assign it to *@errp.
+ * If @errp is NULL, the error is ignored.  Don't bother creating one
+ * then.
+ * If @errp is &error_abort, print a suitable message and abort().
+ * If @errp is anything else, *@errp must be NULL.
+ * The new error's class is ERROR_CLASS_GENERIC_ERROR, and its
+ * human-readable error message is made from printf-style @fmt, ...
  */
-void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
-                     const char *fmt, ...) GCC_FMT_ATTR(4, 5);
-#endif
+#define error_setg(errp, fmt, ...)                              \
+    error_setg_internal((errp), __FILE__, __LINE__, __func__,   \
+                        (fmt), ## __VA_ARGS__)
+void error_setg_internal(Error **errp,
+                         const char *src, int line, const char *func,
+                         const char *fmt, ...)
+    GCC_FMT_ATTR(5, 6);
 
-/**
- * Same as error_set(), but sets a generic error
+/*
+ * Just like error_setg(), with @os_error info added to the message.
+ * If @os_error is non-zero, ": " + strerror(os_error) is appended to
+ * the human-readable error message.
  */
-#define error_setg(errp, fmt, ...) \
-    error_set(errp, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
-#define error_setg_errno(errp, os_error, fmt, ...) \
-    error_set_errno(errp, os_error, ERROR_CLASS_GENERIC_ERROR, \
-                    fmt, ## __VA_ARGS__)
+#define error_setg_errno(errp, os_error, fmt, ...)                      \
+    error_setg_errno_internal((errp), __FILE__, __LINE__, __func__,     \
+                              (os_error), (fmt), ## __VA_ARGS__)
+void error_setg_errno_internal(Error **errp,
+                               const char *fname, int line, const char *func,
+                               int os_error, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
+
 #ifdef _WIN32
-#define error_setg_win32(errp, win32_err, fmt, ...) \
-    error_set_win32(errp, win32_err, ERROR_CLASS_GENERIC_ERROR, \
-                    fmt, ## __VA_ARGS__)
+/*
+ * Just like error_setg(), with @win32_error info added to the message.
+ * If @win32_error is non-zero, ": " + g_win32_error_message(win32_err)
+ * is appended to the human-readable error message.
+ */
+#define error_setg_win32(errp, win32_err, fmt, ...)                     \
+    error_setg_win32_internal((errp), __FILE__, __LINE__, __func__,     \
+                              (win32_err), (fmt), ## __VA_ARGS__)
+void error_setg_win32_internal(Error **errp,
+                               const char *src, int line, const char *func,
+                               int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 #endif
 
-/**
- * Helper for open() errors
+/*
+ * Propagate error object (if any) from @local_err to @dst_errp.
+ * If @local_err is NULL, do nothing (because there's nothing to
+ * propagate).
+ * Else, if @dst_errp is NULL, errors are being ignored.  Free the
+ * error object.
+ * Else, if @dst_errp is &error_abort, print a suitable message and
+ * abort().
+ * Else, if @dst_errp already contains an error, ignore this one: free
+ * the error object.
+ * Else, move the error object from @local_err to *@dst_errp.
+ * On return, @local_err is invalid.
  */
-void error_setg_file_open(Error **errp, int os_errno, const char *filename);
+void error_propagate(Error **dst_errp, Error *local_err);
 
 /*
- * Get the error class of an error object.
+ * Convenience function to report open() failure.
  */
-ErrorClass error_get_class(const Error *err);
+#define error_setg_file_open(errp, os_errno, filename)                  \
+    error_setg_file_open_internal((errp), __FILE__, __LINE__, __func__, \
+                                  (os_errno), (filename))
+void error_setg_file_open_internal(Error **errp,
+                                   const char *src, int line, const char *func,
+                                   int os_errno, const char *filename);
 
-/**
- * Returns an exact copy of the error passed as an argument.
+/*
+ * Return an exact copy of @err.
  */
 Error *error_copy(const Error *err);
 
-/**
- * Get a human readable representation of an error object.
+/*
+ * Free @err.
+ * @err may be NULL.
  */
-const char *error_get_pretty(Error *err);
+void error_free(Error *err);
 
-/**
- * Convenience function to error_report() and free an error object.
+/*
+ * Convenience function to error_report() and free @err.
  */
 void error_report_err(Error *);
 
-/**
- * Propagate an error to an indirect pointer to an error.  This function will
- * always transfer ownership of the error reference and handles the case where
- * dst_err is NULL correctly.  Errors after the first are discarded.
- */
-void error_propagate(Error **dst_errp, Error *local_err);
-
-/**
- * Free an error object.
+/*
+ * Just like error_setg(), except you get to specify the error class.
+ * Note: use of error classes other than ERROR_CLASS_GENERIC_ERROR is
+ * strongly discouraged.
  */
-void error_free(Error *err);
+#define error_set(errp, err_class, fmt, ...)                    \
+    error_set_internal((errp), __FILE__, __LINE__, __func__,    \
+                       (err_class), (fmt), ## __VA_ARGS__)
+void error_set_internal(Error **errp,
+                        const char *src, int line, const char *func,
+                        ErrorClass err_class, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 
-/**
- * If passed to error_set and friends, abort().
+/*
+ * Pass to error_setg() & friends to abort() on error.
  */
-
 extern Error *error_abort;
 
 #endif

qga/vss-win32.c

@@ -150,11 +150,11 @@ void qga_vss_fsfreeze(int *nr_volume, Error **errp, bool freeze)
     const char *func_name = freeze ? "requester_freeze" : "requester_thaw";
     QGAVSSRequesterFunc func;
     ErrorSet errset = {
-        .error_set = (ErrorSetFunc)error_set_win32,
-        .errp = (void **)errp,
-        .err_class = ERROR_CLASS_GENERIC_ERROR
+        .error_setg_win32 = error_setg_win32_internal,
+        .errp = errp,
     };
 
+    g_assert(errp);             /* requester.cpp requires it */
     func = (QGAVSSRequesterFunc)GetProcAddress(provider_lib, func_name);
     if (!func) {
         error_setg_win32(errp, GetLastError(), "failed to load %s from %s",

qga/vss-win32/requester.cpp

@@ -23,10 +23,12 @@
 /* Call QueryStatus every 10 ms while waiting for frozen event */
 #define VSS_TIMEOUT_EVENT_MSEC 10
 
-#define err_set(e, err, fmt, ...) \
-    ((e)->error_set((e)->errp, err, (e)->err_class, fmt, ## __VA_ARGS__))
+#define err_set(e, err, fmt, ...)                                       \
+    ((e)->error_setg_win32((e)->errp, __FILE__, __LINE__, __func__,     \
+                           err, fmt, ## __VA_ARGS__))
+/* Bad idea, works only when (e)->errp != NULL: */
 #define err_is_set(e) ((e)->errp && *(e)->errp)
-
+/* To lift this restriction, error_propagate(), like we do in QEMU code */
 
 /* Handle to VSSAPI.DLL */
 static HMODULE hLib;

qga/vss-win32/requester.h

@@ -20,13 +20,16 @@
 extern "C" {
 #endif
 
+struct Error;
+
 /* Callback to set Error; used to avoid linking glib to the DLL */
-typedef void (*ErrorSetFunc)(void **errp, int win32_err, int err_class,
-                             const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+typedef void (*ErrorSetFunc)(struct Error **errp,
+                             const char *src, int line, const char *func,
+                             int win32_err, const char *fmt, ...)
+    GCC_FMT_ATTR(6, 7);
 typedef struct ErrorSet {
-    ErrorSetFunc error_set;
-    void **errp;
-    int err_class;
+    ErrorSetFunc error_setg_win32;
+    struct Error **errp;        /* restriction: must not be null */
 } ErrorSet;
 
 STDAPI requester_init(void);

util/error.c

@@ -18,14 +18,25 @@ struct Error
 {
     char *msg;
     ErrorClass err_class;
+    const char *src, *func;
+    int line;
 };
 
 Error *error_abort;
 
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+static void error_do_abort(Error *err)
+{
+    fprintf(stderr, "Unexpected error in %s() at %s:%d:\n",
+            err->func, err->src, err->line);
+    error_report_err(err);
+    abort();
+}
+
+static void error_setv(Error **errp,
+                       const char *src, int line, const char *func,
+                       ErrorClass err_class, const char *fmt, va_list ap)
 {
     Error *err;
-    va_list ap;
     int saved_errno = errno;
 
     if (errp == NULL) {
@@ -34,15 +45,14 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     assert(*errp == NULL);
 
     err = g_malloc0(sizeof(*err));
-
-    va_start(ap, fmt);
     err->msg = g_strdup_vprintf(fmt, ap);
-    va_end(ap);
     err->err_class = err_class;
+    err->src = src;
+    err->line = line;
+    err->func = func;
 
     if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
+        error_do_abort(err);
     }
 
     *errp = err;
@@ -50,83 +60,86 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     errno = saved_errno;
 }
 
-void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
-                     const char *fmt, ...)
+void error_set_internal(Error **errp,
+                        const char *src, int line, const char *func,
+                        ErrorClass err_class, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_setv(errp, src, line, func, err_class, fmt, ap);
+    va_end(ap);
+}
+
+void error_setg_internal(Error **errp,
+                         const char *src, int line, const char *func,
+                         const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    va_end(ap);
+}
+
+void error_setg_errno_internal(Error **errp,
+                               const char *src, int line, const char *func,
+                               int os_errno, const char *fmt, ...)
 {
-    Error *err;
-    char *msg1;
     va_list ap;
+    char *msg;
     int saved_errno = errno;
 
     if (errp == NULL) {
         return;
     }
-    assert(*errp == NULL);
-
-    err = g_malloc0(sizeof(*err));
 
     va_start(ap, fmt);
-    msg1 = g_strdup_vprintf(fmt, ap);
-    if (os_errno != 0) {
-        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
-        g_free(msg1);
-    } else {
-        err->msg = msg1;
-    }
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
     va_end(ap);
-    err->err_class = err_class;
 
-    if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
+    if (os_errno != 0) {
+        msg = (*errp)->msg;
+        (*errp)->msg = g_strdup_printf("%s: %s", msg, strerror(os_errno));
+        g_free(msg);
     }
 
-    *errp = err;
-
     errno = saved_errno;
 }
 
-void error_setg_file_open(Error **errp, int os_errno, const char *filename)
+void error_setg_file_open_internal(Error **errp,
+                                   const char *src, int line, const char *func,
+                                   int os_errno, const char *filename)
 {
-    error_setg_errno(errp, os_errno, "Could not open '%s'", filename);
+    error_setg_errno_internal(errp, src, line, func, os_errno,
+                              "Could not open '%s'", filename);
 }
 
 #ifdef _WIN32
 
-void error_set_win32(Error **errp, int win32_err, ErrorClass err_class,
-                     const char *fmt, ...)
+void error_setg_win32_internal(Error **errp,
+                               const char *src, int line, const char *func,
+                               int win32_err, const char *fmt, ...)
 {
-    Error *err;
-    char *msg1;
     va_list ap;
+    char *msg1, *msg2;
 
     if (errp == NULL) {
         return;
     }
-    assert(*errp == NULL);
-
-    err = g_malloc0(sizeof(*err));
 
     va_start(ap, fmt);
-    msg1 = g_strdup_vprintf(fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    va_end(ap);
+
     if (win32_err != 0) {
-        char *msg2 = g_win32_error_message(win32_err);
-        err->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
-                                   (unsigned)win32_err);
+        msg1 = (*errp)->msg;
+        msg2 = g_win32_error_message(win32_err);
+        (*errp)->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
+                                       (unsigned)win32_err);
         g_free(msg2);
         g_free(msg1);
-    } else {
-        err->msg = msg1;
     }
-    va_end(ap);
-    err->err_class = err_class;
-
-    if (errp == &error_abort) {
-        error_report_err(err);
-        abort();
-    }
-
-    *errp = err;
 }
 
 #endif
@@ -169,8 +182,7 @@ void error_free(Error *err)
 void error_propagate(Error **dst_errp, Error *local_err)
 {
     if (local_err && dst_errp == &error_abort) {
-        error_report_err(local_err);
-        abort();
+        error_do_abort(local_err);
     } else if (dst_errp && !*dst_errp) {
         *dst_errp = local_err;
     } else if (local_err) {