diff mbox

[RFC] error: Include hint everywhere

Message ID 20171218144145.13579-1-famz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fam Zheng Dec. 18, 2017, 2:41 p.m. UTC
Previously we only print hint lines if we are in a command line context
or HMP. However QMP errors are also eventually consumed by human and the
hint could help.

Append hint lines already in error_get_pretty() and do as said above
consistently in CLI, HMP and QMP.

Signed-off-by: Fam Zheng <famz@redhat.com>

---

We could drop Error.hint because now it's is unused outside of
error_append_hint, but I tend to see Error.pretty as a cache and 'hint'
is the authentic data.
---
 util/error.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Markus Armbruster Dec. 19, 2017, 2:29 p.m. UTC | #1
Adding Eric for additonal QMP design expertise.

Fam Zheng <famz@redhat.com> writes:

> Previously we only print hint lines if we are in a command line context
> or HMP. However QMP errors are also eventually consumed by human and the
> hint could help.
>
> Append hint lines already in error_get_pretty() and do as said above
> consistently in CLI, HMP and QMP.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>

Problematic.

The intended use of the "hint" feature is to add hints on the *human*
user interface.  These need not make sense for QMP, which has different
syntax.  Please see
http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01991.html
(which I let fall through the cracks, sorry about that).

In practice, we've been using / abusing the feature for other purposes,
too, i.e. hints that do make sense for QMP.

If we decide we want to transmit such hints via QMP, we need to decide
how to (more on that below), and we need to examine the existing hints
one by one to decide whether they make sense for QMP.

If we determine that all of the existing hints make sense for QMP, and
expect that all future hints will, then we can repurpose
error_append_hint().  Else, we need two functions, one for each kind of
hint.

On to QMP design.  Quote qmp-spec.txt:

    2.4.2 error
    -----------

    The format of an error response is:

    { "error": { "class": json-string, "desc": json-string }, "id": json-value }

     Where,

    - The "class" member contains the error class name (eg. "GenericError")
    - The "desc" member is a human-readable error message. Clients should
      not attempt to parse this message.
    - The "id" member contains the transaction identification associated with
      the command execution if issued by the Client

Your patch changes "desc" from a short(ish) message without newline to a
multi-line message that may or may not end with a newline (I think).
Is that a good idea?

The conservative way to add hints is a new optional member.
Fam Zheng Dec. 20, 2017, 6:30 a.m. UTC | #2
On Tue, 12/19 15:29, Markus Armbruster wrote:
> Adding Eric for additonal QMP design expertise.
> 
> Fam Zheng <famz@redhat.com> writes:
> 
> > Previously we only print hint lines if we are in a command line context
> > or HMP. However QMP errors are also eventually consumed by human and the
> > hint could help.
> >
> > Append hint lines already in error_get_pretty() and do as said above
> > consistently in CLI, HMP and QMP.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> Problematic.
> 
> The intended use of the "hint" feature is to add hints on the *human*
> user interface.  These need not make sense for QMP, which has different
> syntax.  Please see
> http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01991.html
> (which I let fall through the cracks, sorry about that).
> 
> In practice, we've been using / abusing the feature for other purposes,
> too, i.e. hints that do make sense for QMP.
> 
> If we decide we want to transmit such hints via QMP, we need to decide
> how to (more on that below), and we need to examine the existing hints
> one by one to decide whether they make sense for QMP.
> 
> If we determine that all of the existing hints make sense for QMP, and
> expect that all future hints will, then we can repurpose
> error_append_hint().  Else, we need two functions, one for each kind of
> hint.
> 
> On to QMP design.  Quote qmp-spec.txt:
> 
>     2.4.2 error
>     -----------
> 
>     The format of an error response is:
> 
>     { "error": { "class": json-string, "desc": json-string }, "id": json-value }
> 
>      Where,
> 
>     - The "class" member contains the error class name (eg. "GenericError")
>     - The "desc" member is a human-readable error message. Clients should
>       not attempt to parse this message.
>     - The "id" member contains the transaction identification associated with
>       the command execution if issued by the Client
> 
> Your patch changes "desc" from a short(ish) message without newline to a
> multi-line message that may or may not end with a newline (I think).
> Is that a good idea?

Hmm, I didn't pay attention to the ending '\n', you are right we should keep it
consistent.

Multiline is not a big problem IMO. Because, I'm inclined to think that for
GenericError errors, desc are not for machines; the desc content is not part of
the API and we are free to update it.

That said, the case where my patch would be inappropriate is when the error
comes from a libvirt misconfiguration but the error message is QEMU-specific and
doesn't make sense in a libvirt (abstraction) context.

So you are probably right we still need to distinguish them in the QMP level.

Maybe the right thing to do is just convert any error_append_hint() to an
updated error_setg() and already include the "hint" in the error message proper.
So that we can document that error_append_hint() is specifically used for the
case said above, for example.

> 
> The conservative way to add hints is a new optional member.

I don't like that because it leaves the decision for how to handle the optional
information to the management layer, which, again, may not have the right
information (whether displaying the message to users makes any sense).

Fam
Markus Armbruster Dec. 20, 2017, 9:05 a.m. UTC | #3
Fam Zheng <famz@redhat.com> writes:

> On Tue, 12/19 15:29, Markus Armbruster wrote:
>> Adding Eric for additonal QMP design expertise.
>> 
>> Fam Zheng <famz@redhat.com> writes:
>> 
>> > Previously we only print hint lines if we are in a command line context
>> > or HMP. However QMP errors are also eventually consumed by human and the
>> > hint could help.
>> >
>> > Append hint lines already in error_get_pretty() and do as said above
>> > consistently in CLI, HMP and QMP.
>> >
>> > Signed-off-by: Fam Zheng <famz@redhat.com>
>> 
>> Problematic.
>> 
>> The intended use of the "hint" feature is to add hints on the *human*
>> user interface.  These need not make sense for QMP, which has different
>> syntax.  Please see
>> http://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01991.html
>> (which I let fall through the cracks, sorry about that).
>> 
>> In practice, we've been using / abusing the feature for other purposes,
>> too, i.e. hints that do make sense for QMP.
>> 
>> If we decide we want to transmit such hints via QMP, we need to decide
>> how to (more on that below), and we need to examine the existing hints
>> one by one to decide whether they make sense for QMP.
>> 
>> If we determine that all of the existing hints make sense for QMP, and
>> expect that all future hints will, then we can repurpose
>> error_append_hint().  Else, we need two functions, one for each kind of
>> hint.
>> 
>> On to QMP design.  Quote qmp-spec.txt:
>> 
>>     2.4.2 error
>>     -----------
>> 
>>     The format of an error response is:
>> 
>>     { "error": { "class": json-string, "desc": json-string }, "id": json-value }
>> 
>>      Where,
>> 
>>     - The "class" member contains the error class name (eg. "GenericError")
>>     - The "desc" member is a human-readable error message. Clients should
>>       not attempt to parse this message.
>>     - The "id" member contains the transaction identification associated with
>>       the command execution if issued by the Client
>> 
>> Your patch changes "desc" from a short(ish) message without newline to a
>> multi-line message that may or may not end with a newline (I think).
>> Is that a good idea?
>
> Hmm, I didn't pay attention to the ending '\n', you are right we should keep it
> consistent.
>
> Multiline is not a big problem IMO. Because, I'm inclined to think that for
> GenericError errors, desc are not for machines; the desc content is not part of
> the API and we are free to update it.

"desc" being a short(ish) message without newline is de facto ABI.
A management application that logs messages could well be upset by a
switch to multiline.

A simple example is a log file consisting of lines with a fixed prefix
followed by free-form text.  Code writing such a log needs to be
prepared for newlines in free-form text, or else you'll get things like

    Dec 20 09:20:54 Some log entry
    Dec 20 09:24:14 Error message
    First line of hint
    Second line of hint
    Dec 20 10:01:05 Some other log entry

Two hint lines do not conform to the format.  Except when their text
matches the fixed format, but that's even worse, because then you can't
even identify the start of messages reliably.

> That said, the case where my patch would be inappropriate is when the error
> comes from a libvirt misconfiguration but the error message is QEMU-specific and
> doesn't make sense in a libvirt (abstraction) context.
>
> So you are probably right we still need to distinguish them in the QMP level.
>
> Maybe the right thing to do is just convert any error_append_hint() to an
> updated error_setg() and already include the "hint" in the error message proper.
> So that we can document that error_append_hint() is specifically used for the
> case said above, for example.

I'm not sure I understand what you're proposing to do.

>> The conservative way to add hints is a new optional member.
>
> I don't like that because it leaves the decision for how to handle the optional
> information to the management layer, which, again, may not have the right
> information (whether displaying the message to users makes any sense).

Whether we encode error message and hint in a single string separated by
the first newline, or in two strings doesn't really change this problem,
does it?
diff mbox

Patch

diff --git a/util/error.c b/util/error.c
index 3efdd69162..f42ae248b4 100644
--- a/util/error.c
+++ b/util/error.c
@@ -24,6 +24,7 @@  struct Error
     const char *src, *func;
     int line;
     GString *hint;
+    GString *pretty;
 };
 
 Error *error_abort;
@@ -166,6 +167,11 @@  void error_append_hint(Error **errp, const char *fmt, ...)
     g_string_append_vprintf(err->hint, fmt, ap);
     va_end(ap);
 
+    if (!err->pretty) {
+        err->pretty = g_string_new(NULL);
+    }
+    g_string_printf(err->pretty, "%s\n%s", err->msg, err->hint->str);
+
     errno = saved_errno;
 }
 
@@ -206,8 +212,10 @@  Error *error_copy(const Error *err)
     err_new->src = err->src;
     err_new->line = err->line;
     err_new->func = err->func;
+    assert(!!err->hint == !!err->pretty);
     if (err->hint) {
         err_new->hint = g_string_new(err->hint->str);
+        err_new->pretty = g_string_new(err->pretty->str);
     }
 
     return err_new;
@@ -220,24 +228,24 @@  ErrorClass error_get_class(const Error *err)
 
 const char *error_get_pretty(const Error *err)
 {
-    return err->msg;
+    if (err->pretty) {
+        assert(err->hint);
+        return err->pretty->str;
+    } else {
+        assert(!err->hint);
+        return err->msg;
+    }
 }
 
 void error_report_err(Error *err)
 {
     error_report("%s", error_get_pretty(err));
-    if (err->hint) {
-        error_printf_unless_qmp("%s", err->hint->str);
-    }
     error_free(err);
 }
 
 void warn_report_err(Error *err)
 {
     warn_report("%s", error_get_pretty(err));
-    if (err->hint) {
-        error_printf_unless_qmp("%s", err->hint->str);
-    }
     error_free(err);
 }
 
@@ -268,6 +276,7 @@  void error_free(Error *err)
         g_free(err->msg);
         if (err->hint) {
             g_string_free(err->hint, true);
+            g_string_free(err->pretty, true);
         }
         g_free(err);
     }