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

[Fam Zheng <famz@redhat.com>]

On Fri, 06/01 15:00, Daniel P. Berrangé wrote:
> 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.

I have no inclination to changing any other hints in the block layer. See
below..

> 
> 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.

Yes I think that is the plan. And that's the reason for exceptional cases like
this patch, for older libvirts (which of course is subjective).

Fam