[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/7] qdev-monitor: Convert qbus_find() to Error
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 5/7] qdev-monitor: Convert qbus_find() to Error |
Date: |
Tue, 09 Jun 2015 08:07:28 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 06/08/2015 12:57 PM, Markus Armbruster wrote:
>> As usual, the conversion breaks printing explanatory messages after
>> the error: actual printing of the error gets delayed, so the
>> explanations precede rather than follow it.
>>
>> Pity. Disable them for now. See also commit 7216ae3.
>
> Could we add some sort of error_append_hmp_hint() that adds additional
> messages to an existing error object, for use when the error will be
> printed via HMP but is a no-op for QMP? (and make it callable more than
> once, since qbus_list_dev() uses error_printf() in that role more than once)
Should work. Should get its own series, of course. Volunteer welcome
:)
> But that can be a later patch, this one is fine as-is for following
> existing practice.
Agree.
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>> include/qapi/qmp/qerror.h | 3 ---
>> qdev-monitor.c | 30 +++++++++++++++++++-----------
>> 2 files changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index e567339..6468e40 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -43,9 +43,6 @@ void qerror_report_err(Error *err);
>> #define QERR_BUS_NO_HOTPLUG \
>> ERROR_CLASS_GENERIC_ERROR, "Bus '%s' does not support hotplugging"
>>
>> -#define QERR_BUS_NOT_FOUND \
>> - ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found"
>
> Might want to mention one less baroque macro in qerror.h in the commit
> message as an intentional part of the conversion.
Could append:
While there, eliminate QERR_BUS_NOT_FOUND.
>> @@ -475,14 +479,15 @@ static BusState *qbus_find(const char *path)
>> break;
>> }
>> if (dev->num_child_bus) {
>> - qerror_report(ERROR_CLASS_GENERIC_ERROR,
>> - "Device '%s' has multiple child busses",
>> elem);
>> + error_setg(errp, "Device '%s' has multiple child busses",
>
> Stupid spell-check on my mailer is flagging 'busses' as a typo, even
> though dictionary.com says both spellings are acceptable. Other sources
> prefer 'buses' and say 'busses' is out of favor:
> http://grammarist.com/spelling/buses-busses/
If I have to respin anyway, I can fold in s/busses/buses/, and amend
While there, eliminate QERR_BUS_NOT_FOUND, and clean up unusual
spelling in the error message.
> You could always skirt the confusion by creative wording like "has
> multiple bus children".
Nah, we just spell like the dictionary says we should.
> But it is a pre-existing issue [if an issue at
> all], so I don't care enough to make it hold up this patch.
>
> Reviewed-by: Eric Blake <address@hidden>
Thanks!
- [Qemu-devel] [PATCH 6/7] qdev-monitor: Propagate errors through set_property(), (continued)
- [Qemu-devel] [PATCH 6/7] qdev-monitor: Propagate errors through set_property(), Markus Armbruster, 2015/06/08
- [Qemu-devel] [PATCH 2/7] qdev: Un-deprecate qdev_init_nofail(), Markus Armbruster, 2015/06/08
- [Qemu-devel] [PATCH 4/7] qdev-monitor: Fix check for full bus, Markus Armbruster, 2015/06/08
- [Qemu-devel] [PATCH 3/7] qdev-monitor: Stop error avalance in qbus_find_recursive(), Markus Armbruster, 2015/06/08
- [Qemu-devel] [PATCH 5/7] qdev-monitor: Convert qbus_find() to Error, Markus Armbruster, 2015/06/08
- [Qemu-devel] [PATCH 1/7] qdev: Deprecated qdev_init() is finally unused, drop, Markus Armbruster, 2015/06/08
- [Qemu-devel] [PATCH 7/7] qdev-monitor: Propagate errors through qdev_device_add(), Markus Armbruster, 2015/06/08