[Qemu-devel] [PATCH v4] hmp: Allow for error message hints on HMP

[Qemu-devel] [PATCH v4] hmp: Allow for error message hints on HMP

[Eric Blake]

Commits 7216ae3d and d2828429 disabled some error message hints,
all because a change to use modern error reporting meant that the
hint would be output prior to the actual error.  Fix this by making
hints a first-class member of Error.

For example, we are now back to the pleasant:

 $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
 qemu-system-x86_64: --chardev null,id=,: Parameter 'id' expects an identifier
 Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---

v4: add R-b, rebase to latest qemu.git
v3: don't crash when freeing error without hint [Eric]
v2: use GString instead of rolling our own O(n^2) string concast [Paolo]

 include/qapi/error.h |  7 +++++++
 qdev-monitor.c       | 42 ++++++++++++++----------------------------
 util/error.c         | 32 ++++++++++++++++++++++++++++++++
 util/qemu-option.c   | 11 ++++-------
 4 files changed, 57 insertions(+), 35 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 426d5ea..d7878c3 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -155,6 +155,13 @@ void error_setg_win32_internal(Error **errp,
  */
 void error_propagate(Error **dst_errp, Error *local_err);

+/**
+ * Append a printf-style human-readable explanation to an existing error.
+ * May be called multiple times, and safe if @errp is NULL.
+ */
+void error_append_hint(Error **errp, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
 /*
  * Convenience function to report open() failure.
  */
diff --git a/qdev-monitor.c b/qdev-monitor.c
index f9e2d62..0bf7f83 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -289,37 +289,35 @@ static Object *qdev_get_peripheral_anon(void)
     return dev;
 }

-#if 0 /* conversion from qerror_report() to error_set() broke their use */
-static void qbus_list_bus(DeviceState *dev)
+static void qbus_list_bus(DeviceState *dev, Error **errp)
 {
     BusState *child;
     const char *sep = " ";

-    error_printf("child buses at \"%s\":",
-                 dev->id ? dev->id : object_get_typename(OBJECT(dev)));
+    error_append_hint(errp, "child buses at \"%s\":",
+                      dev->id ? dev->id : object_get_typename(OBJECT(dev)));
     QLIST_FOREACH(child, &dev->child_bus, sibling) {
-        error_printf("%s\"%s\"", sep, child->name);
+        error_append_hint(errp, "%s\"%s\"", sep, child->name);
         sep = ", ";
     }
-    error_printf("\n");
 }

-static void qbus_list_dev(BusState *bus)
+static void qbus_list_dev(BusState *bus, Error **errp)
 {
     BusChild *kid;
     const char *sep = " ";

-    error_printf("devices at \"%s\":", bus->name);
+    error_append_hint(errp, "devices at \"%s\":", bus->name);
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
         DeviceState *dev = kid->child;
-        error_printf("%s\"%s\"", sep, object_get_typename(OBJECT(dev)));
-        if (dev->id)
-            error_printf("/\"%s\"", dev->id);
+        error_append_hint(errp, "%s\"%s\"", sep,
+                          object_get_typename(OBJECT(dev)));
+        if (dev->id) {
+            error_append_hint(errp, "/\"%s\"", dev->id);
+        }
         sep = ", ";
     }
-    error_printf("\n");
 }
-#endif

 static BusState *qbus_find_bus(DeviceState *dev, char *elem)
 {
@@ -461,11 +459,7 @@ static BusState *qbus_find(const char *path, Error **errp)
         if (!dev) {
             error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                       "Device '%s' not found", elem);
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-            if (!monitor_cur_is_qmp()) {
-                qbus_list_dev(bus);
-            }
-#endif
+            qbus_list_dev(bus, errp);
             return NULL;
         }

@@ -483,11 +477,7 @@ static BusState *qbus_find(const char *path, Error **errp)
             if (dev->num_child_bus) {
                 error_setg(errp, "Device '%s' has multiple child buses",
                            elem);
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-                if (!monitor_cur_is_qmp()) {
-                    qbus_list_bus(dev);
-                }
-#endif
+                qbus_list_bus(dev, errp);
             } else {
                 error_setg(errp, "Device '%s' has no child bus", elem);
             }
@@ -503,11 +493,7 @@ static BusState *qbus_find(const char *path, Error **errp)
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
             error_setg(errp, "Bus '%s' not found", elem);
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-            if (!monitor_cur_is_qmp()) {
-                qbus_list_bus(dev);
-            }
-#endif
+            qbus_list_bus(dev, errp);
             return NULL;
         }
     }
diff --git a/util/error.c b/util/error.c
index cdb726c..9dd474f 100644
--- a/util/error.c
+++ b/util/error.c
@@ -20,6 +20,7 @@ struct Error
     ErrorClass err_class;
     const char *src, *func;
     int line;
+    GString *hint;
 };

 Error *error_abort;
@@ -115,6 +116,28 @@ void error_setg_file_open_internal(Error **errp,
                               "Could not open '%s'", filename);
 }

+void error_append_hint(Error **errp, const char *fmt, ...)
+{
+    va_list ap;
+    int saved_errno = errno;
+    Error *err;
+
+    if (!errp) {
+        return;
+    }
+    err = *errp;
+    assert(err && errp != &error_abort);
+
+    if (!err->hint) {
+        err->hint = g_string_new(NULL);
+    }
+    va_start(ap, fmt);
+    g_string_append_vprintf(err->hint, fmt, ap);
+    va_end(ap);
+
+    errno = saved_errno;
+}
+
 #ifdef _WIN32

 void error_setg_win32_internal(Error **errp,
@@ -151,6 +174,9 @@ Error *error_copy(const Error *err)
     err_new = g_malloc0(sizeof(*err));
     err_new->msg = g_strdup(err->msg);
     err_new->err_class = err->err_class;
+    if (err->hint) {
+        err_new->hint = g_string_new(err->hint->str);
+    }

     return err_new;
 }
@@ -168,6 +194,9 @@ const char *error_get_pretty(Error *err)
 void error_report_err(Error *err)
 {
     error_report("%s", error_get_pretty(err));
+    if (err->hint) {
+        error_printf_unless_qmp("%s\n", err->hint->str);
+    }
     error_free(err);
 }

@@ -175,6 +204,9 @@ void error_free(Error *err)
 {
     if (err) {
         g_free(err->msg);
+        if (err->hint) {
+            g_string_free(err->hint, true);
+        }
         g_free(err);
     }
 }
diff --git a/util/qemu-option.c b/util/qemu-option.c
index efd6f02..6c76ed2 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -205,10 +205,8 @@ void parse_option_size(const char *name, const char *value,
             break;
         default:
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
-                    "kilobytes, megabytes, gigabytes and terabytes.\n");
-#endif
+            error_append_hint(errp, "You may use k, M, G or T suffixes for "
+                    "kilobytes, megabytes, gigabytes and terabytes.");
             return;
         }
     } else {
@@ -648,9 +646,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
         if (!id_wellformed(id)) {
             error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
                        "an identifier");
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-            error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
-#endif
+            error_append_hint(errp, "Identifiers consist of letters, digits, "
+                              "'-', '.', '_', starting with a letter.");
             return NULL;
         }
         opts = qemu_opts_find(list, id);
-- 
2.4.3

[Qemu-devel] [PATCH 2/1] error: Copy location information in error_copy()

[Eric Blake]

Commit 1e9b65bb forgot to propagate source information to copied
errors.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

I noticed this while rebasing my patch (as in 'why did I not
get a merge conflict where I expected one?'); of course we
could apply this one first, but swapping the patch order implies
even more rebasing :)

 util/error.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/util/error.c b/util/error.c
index 9dd474f..b1eb8a2 100644
--- a/util/error.c
+++ b/util/error.c
@@ -174,6 +174,9 @@ Error *error_copy(const Error *err)
     err_new = g_malloc0(sizeof(*err));
     err_new->msg = g_strdup(err->msg);
     err_new->err_class = err->err_class;
+    err_new->src = err->src;
+    err_new->line = err->line;
+    err_new->func = err->func;
     if (err->hint) {
         err_new->hint = g_string_new(err->hint->str);
     }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v4] hmp: Allow for error message hints on HMP

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commits 7216ae3d and d2828429 disabled some error message hints,
> all because a change to use modern error reporting meant that the
> hint would be output prior to the actual error.  Fix this by making
> hints a first-class member of Error.
>
> For example, we are now back to the pleasant:
>
>  $ qemu-system-x86_64 --nodefaults -S --vnc :0 --chardev null,id=,
>  qemu-system-x86_64: --chardev null,id=,: Parameter 'id' expects an identifier
>  Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>
> v4: add R-b, rebase to latest qemu.git
> v3: don't crash when freeing error without hint [Eric]
> v2: use GString instead of rolling our own O(n^2) string concast [Paolo]
>
>  include/qapi/error.h |  7 +++++++
>  qdev-monitor.c       | 42 ++++++++++++++----------------------------
>  util/error.c         | 32 ++++++++++++++++++++++++++++++++
>  util/qemu-option.c   | 11 ++++-------
>  4 files changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 426d5ea..d7878c3 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -155,6 +155,13 @@ void error_setg_win32_internal(Error **errp,
>   */
>  void error_propagate(Error **dst_errp, Error *local_err);
>
> +/**
> + * Append a printf-style human-readable explanation to an existing error.
> + * May be called multiple times, and safe if @errp is NULL.
> + */
> +void error_append_hint(Error **errp, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +
>  /*
>   * Convenience function to report open() failure.
>   */
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index f9e2d62..0bf7f83 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -289,37 +289,35 @@ static Object *qdev_get_peripheral_anon(void)
>      return dev;
>  }
>
> -#if 0 /* conversion from qerror_report() to error_set() broke their use */
> -static void qbus_list_bus(DeviceState *dev)
> +static void qbus_list_bus(DeviceState *dev, Error **errp)
>  {
>      BusState *child;
>      const char *sep = " ";
>
> -    error_printf("child buses at \"%s\":",
> -                 dev->id ? dev->id : object_get_typename(OBJECT(dev)));
> +    error_append_hint(errp, "child buses at \"%s\":",
> +                      dev->id ? dev->id : object_get_typename(OBJECT(dev)));
>      QLIST_FOREACH(child, &dev->child_bus, sibling) {
> -        error_printf("%s\"%s\"", sep, child->name);
> +        error_append_hint(errp, "%s\"%s\"", sep, child->name);
>          sep = ", ";
>      }
> -    error_printf("\n");
>  }
>
> -static void qbus_list_dev(BusState *bus)
> +static void qbus_list_dev(BusState *bus, Error **errp)
>  {
>      BusChild *kid;
>      const char *sep = " ";
>
> -    error_printf("devices at \"%s\":", bus->name);
> +    error_append_hint(errp, "devices at \"%s\":", bus->name);
>      QTAILQ_FOREACH(kid, &bus->children, sibling) {
>          DeviceState *dev = kid->child;
> -        error_printf("%s\"%s\"", sep, object_get_typename(OBJECT(dev)));
> -        if (dev->id)
> -            error_printf("/\"%s\"", dev->id);
> +        error_append_hint(errp, "%s\"%s\"", sep,
> +                          object_get_typename(OBJECT(dev)));
> +        if (dev->id) {
> +            error_append_hint(errp, "/\"%s\"", dev->id);
> +        }
>          sep = ", ";
>      }
> -    error_printf("\n");
>  }
> -#endif
>
>  static BusState *qbus_find_bus(DeviceState *dev, char *elem)
>  {
> @@ -461,11 +459,7 @@ static BusState *qbus_find(const char *path, Error **errp)
>          if (!dev) {
>              error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>                        "Device '%s' not found", elem);
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            if (!monitor_cur_is_qmp()) {
> -                qbus_list_dev(bus);
> -            }
> -#endif
> +            qbus_list_dev(bus, errp);
>              return NULL;
>          }
>
> @@ -483,11 +477,7 @@ static BusState *qbus_find(const char *path, Error **errp)
>              if (dev->num_child_bus) {
>                  error_setg(errp, "Device '%s' has multiple child buses",
>                             elem);
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -                if (!monitor_cur_is_qmp()) {
> -                    qbus_list_bus(dev);
> -                }
> -#endif
> +                qbus_list_bus(dev, errp);
>              } else {
>                  error_setg(errp, "Device '%s' has no child bus", elem);
>              }
> @@ -503,11 +493,7 @@ static BusState *qbus_find(const char *path, Error **errp)
>          bus = qbus_find_bus(dev, elem);
>          if (!bus) {
>              error_setg(errp, "Bus '%s' not found", elem);
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            if (!monitor_cur_is_qmp()) {
> -                qbus_list_bus(dev);
> -            }
> -#endif
> +            qbus_list_bus(dev, errp);
>              return NULL;
>          }
>      }
> diff --git a/util/error.c b/util/error.c
> index cdb726c..9dd474f 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -20,6 +20,7 @@ struct Error
>      ErrorClass err_class;
>      const char *src, *func;
>      int line;
> +    GString *hint;
>  };
>
>  Error *error_abort;
> @@ -115,6 +116,28 @@ void error_setg_file_open_internal(Error **errp,
>                                "Could not open '%s'", filename);
>  }
>
> +void error_append_hint(Error **errp, const char *fmt, ...)
> +{
> +    va_list ap;
> +    int saved_errno = errno;
> +    Error *err;
> +
> +    if (!errp) {
> +        return;
> +    }
> +    err = *errp;
> +    assert(err && errp != &error_abort);
> +
> +    if (!err->hint) {
> +        err->hint = g_string_new(NULL);
> +    }
> +    va_start(ap, fmt);
> +    g_string_append_vprintf(err->hint, fmt, ap);
> +    va_end(ap);
> +
> +    errno = saved_errno;
> +}
> +
>  #ifdef _WIN32
>
>  void error_setg_win32_internal(Error **errp,
> @@ -151,6 +174,9 @@ Error *error_copy(const Error *err)
>      err_new = g_malloc0(sizeof(*err));
>      err_new->msg = g_strdup(err->msg);
>      err_new->err_class = err->err_class;
> +    if (err->hint) {
> +        err_new->hint = g_string_new(err->hint->str);
> +    }
>
>      return err_new;
>  }
> @@ -168,6 +194,9 @@ const char *error_get_pretty(Error *err)
>  void error_report_err(Error *err)
>  {
>      error_report("%s", error_get_pretty(err));
> +    if (err->hint) {
> +        error_printf_unless_qmp("%s\n", err->hint->str);
> +    }
>      error_free(err);
>  }
>
> @@ -175,6 +204,9 @@ void error_free(Error *err)
>  {
>      if (err) {
>          g_free(err->msg);
> +        if (err->hint) {
> +            g_string_free(err->hint, true);
> +        }
>          g_free(err);
>      }
>  }

Once again, GLib demonstrates its lack of taste.

> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index efd6f02..6c76ed2 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -205,10 +205,8 @@ void parse_option_size(const char *name, const char *value,
>              break;
>          default:
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
> -                    "kilobytes, megabytes, gigabytes and terabytes.\n");
> -#endif
> +            error_append_hint(errp, "You may use k, M, G or T suffixes for "
> +                    "kilobytes, megabytes, gigabytes and terabytes.");
>              return;
>          }
>      } else {
> @@ -648,9 +646,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>          if (!id_wellformed(id)) {
>              error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
>                         "an identifier");
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
> -#endif
> +            error_append_hint(errp, "Identifiers consist of letters, digits, "
> +                              "'-', '.', '_', starting with a letter.");
>              return NULL;
>          }
>          opts = qemu_opts_find(list, id);

I'll take this through my tree.  Thanks!

Re: [Qemu-devel] [PATCH 2/1] error: Copy location information in error_copy()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commit 1e9b65bb forgot to propagate source information to copied
> errors.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>
> I noticed this while rebasing my patch (as in 'why did I not
> get a merge conflict where I expected one?'); of course we
> could apply this one first, but swapping the patch order implies
> even more rebasing :)
>
>  util/error.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/util/error.c b/util/error.c
> index 9dd474f..b1eb8a2 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -174,6 +174,9 @@ Error *error_copy(const Error *err)
>      err_new = g_malloc0(sizeof(*err));
>      err_new->msg = g_strdup(err->msg);
>      err_new->err_class = err->err_class;
> +    err_new->src = err->src;
> +    err_new->line = err->line;
> +    err_new->func = err->func;
>      if (err->hint) {
>          err_new->hint = g_string_new(err->hint->str);
>      }

Escaped review, but not for long :)

I'll take this through my tree.  Thanks!