[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH] file-posix: Consolidate the lockin
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH] file-posix: Consolidate the locking error message |
Date: |
Fri, 1 Jun 2018 22:17:21 +0800 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
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