Re: [Qemu-devel] [PATCH 5/7] qdev-monitor: Convert qbus_find() to Error

[Markus Armbruster]

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!