Markus Armbruster <[email protected]> writes:
> Fabiano Rosas <[email protected]> writes:
>
>> Fabiano Rosas <[email protected]> writes:
>>
>>> Markus Armbruster <[email protected]> writes:
>
> [...]
>
>>>> I think a hmp_report_error(MonitorHMP *hmp, const char *fmt, ...) would
>>>> be nice to have.
>>>>
>>>
>>> I'll look into it.
>>>
>>
>> I see that hmp_handle_error() takes an hmp argument that's unused and
>
> Unused since commit 193227f9e5 (error: Use error_report_err() instead of
> monitor_printf(), 2015-12-18).
>
> I doubt it's worth removing.
>
>> also that callers often do the redundant:
>>
>> if (err) {
>> hmp_handle_error(err);
>> }
>
> Would be nice to clean this up.
>
Yep, I've a patch for it.
>> But aside from those I don't see other improvements to be made, many
>> callsites of hmp_handle_error() re-use the Error for other calls,
>> e.g. block/monitor/block-hmp-cmds.c:
>>
>> Error *err = NULL;
>> ...
>> if (!qdict_get_try_str(qdict, "node-name")) {
>> qobject_unref(qdict);
>> error_setg(&err, "'node-name' needs to be specified");
>> goto out;
>> }
>>
>> BlockDriverState *bs = bds_tree_init(qdict, &err);
>> if (!bs) {
>> goto out;
>> }
>>
>> bdrv_set_monitor_owned(bs);
>> out:
>> qemu_opts_del(opts);
>> hmp_handle_error(err);
>>
>> So having another helper I think would just create confusion as
>> error_setg() is already a standard way of creating an error.
>
> We have both error_report() and error_report_err(), and it doesn't lead
> to confusion as far as I can tell.
>
They're not analogous, hmp_handle_error() it's more versatile than
error_report_err() because it can cope with err=NULL:
// this uses only one helper for the error
pseudo1() {
err = NULL;
ret = afunc();
if (ret < 0) {
error_setg(&err, "fail");
goto out;
}
another_func(&err);
if (hmp_handle_error(err)) {
return;
}
yet_another_func(&err);
out:
hmp_handle_error(err);
}
vs.
// this needs two different helpers
pseudo2() {
err = NULL;
ret = afunc();
if (ret < 0) {
hmp_error_report("fail");
return;
}
another_func(&err);
if (hmp_handle_error(err)) {
return;
}
yet_another_func(&err);
hmp_handle_error(err);
}
> Whether doing the same for HMP error reporting would be an improvement
> is not obvious. Let's move on.
>
Sure
> [...]
