Re: [Qemu-devel] [PATCH] file-posix: Consolidate the locking error message

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Jun 01, 2018 at 09:33:59PM +0800, Fam Zheng wrote:
> On Fri, 06/01 13:43, Daniel P. Berrangé wrote:
> > On Fri, Jun 01, 2018 at 05:18:35PM +0800, Fam Zheng wrote:
> > > When hot-plugging a block device fails due to image locking errors,
> > > users won't see the helpful 'Is another process using the image?'
> > > message in QMP because currently the error hint is not carried over
> > > there.
> > > 
> > > Even though extending QMP to include hint is a conceivably easy task,
> > > Libvirt will need some change to consume that data.
> > > 
> > > Before that is fully sorted out, let's just do the easy fix by joining
> > > the two lines.
> > 
> > There are many places in QEMU which uses error hints and these are all
> > invisible to libvirt. Arbitrarily picking one hint to remove, while
> > leaving everything else unfixed is not a very satisfactory approach.
> > 
> > If QEMU passes the hint in QMP, it is trivial for libvirt to be changed
> > to append the hint when reporting its own error message, so can we just
> > focus on fixing the root cause instead of special casing file-posix.c
> 
> The plan was to work on the QMP change in parallel, while this simple patch can
> mitigate the confusion caused by the relatively vague message (the text itself
> is going a bit on the cryptic side for people who don't know QEMU internals).

I still don't think that it makes sense to remove the use of hints in the
block layer.

If we don't care about improved error messages for existing libvirt
versions, we can just add a 'hint' field in QMP and let new libvirt
use that:

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 935f9e159c..82eb823f1f 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -132,9 +132,15 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
 
 QObject *qmp_build_error_object(Error *err)
 {
-    return qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
-                              QapiErrorClass_str(error_get_class(err)),
-                              error_get_pretty(err));
+    const char *hint = error_get_hint(err);
+    if (hint)
+        return qobject_from_jsonf("{ 'class': %s, 'desc': %s, 'hint': %s }",
+                                  QapiErrorClass_str(error_get_class(err)),
+                                  error_get_pretty(err), hint);
+    else
+        return qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
+                                  QapiErrorClass_str(error_get_class(err)),
+                                  error_get_pretty(err));
 }
 
 /*


there's no error_get_hint method right now, but its impl is essentially
the same as error_get_pretty.


If, however, we want to get better error messages for existing libvirt,
then, we should do:

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 935f9e159c..bf6f92375a 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -132,9 +132,18 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
 
 QObject *qmp_build_error_object(Error *err)
 {
-    return qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
-                              QapiErrorClass_str(error_get_class(err)),
-                              error_get_pretty(err));
+    const char *hint = error_get_hint(err);
+    const char *msg;
+    if (hint) {
+        msg = g_strdup_printf("%s: %s", error_get_pretty(), hint);
+    } else {
+        msg = g_strdup(error_get_pretty());
+    }
+    QObject *ret = qobject_from_jsonf("{ 'class': %s, 'desc': %s }",
+                                      QapiErrorClass_str(error_get_class(err)),
+                                      msg);
+    g_free(msg);
+    return ret;
 }
 
 /*

Personally I'd just go for the first case and only care about new libvirts.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|