Re: [PATCH 12/12] error: Kill @error_warn

[Markus Armbruster <armbru@redhat.com>]

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> On 2025/08/08 17:08, Markus Armbruster wrote:
>> We added @error_warn some two years ago in commit 3ffef1a55ca (error:
>> add global &error_warn destination).  It has multiple issues:
>>
>> * error.h's big comment was not updated for it.
>>
>> * Function contracts were not updated for it.
>>
>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>>    error_prepend() and such.  These crash on @error_warn, as pointed
>>    out by Akihiko Odaki.
>>
>> All fixable.  However, after more than two years, we had just of 15
>> uses, of which the last few patches removed eight as unclean or
>> otherwise undesirable.  I didn't look closely enough at the remaining
>> seven to decide whether they are desirable or not.
>>
>> I don't think this feature earns its keep.  Drop it.
>
> I want to note that the following patch series temporarily use &error_warn during its conversion to add errp parameters:
> https://lore.kernel.org/qemu-devel/20250808-propagate_tpm_error-v10-0-3e81a1d419b2@redhat.com/
> ("[PATCH v10 00/27] migration: propagate vTPM errors using Error objects")
>
> I think this series needs to be rebased on top of the migration change.

Thanks for the heads-up.

> I'm not sure if seven uses are insufficient to keep it.
>
> I also have a general impression that perhaps a special error destination for error_report_err() is more useful. Today, there are many functions use Error, but there are also functions that still follow old error handling patterns. When legacy functions call functions with Error, a common pattern is to use error_report_err() and return -1.

You mean like &error_fatal less the exit(1)?

> "[PATCH 01/12] monitor: Clean up HMP gdbserver error reporting" and "[PATCH 09/12] ui/pixman: Consistent error handling in qemu_pixman_shareable_free()" are examples that will benefit from error_report_err() as an error destination. The migration patch series I mentioned earlier can also use one.
>
> Perhaps it is nicer if there is an infrastructure shared by the special destinations. In particular, we can have common solutions for the three problems you pointed out:
>
>> * error.h's big comment was not updated for it.
>>
>> * Function contracts were not updated for it.
>
> For these two problems, they can refer to "special error destinations" instead of listing them, and delegate explanations of them to corresponding ones.
>
>>
>> * ERRP_GUARD() is unaware of @error_warn, and fails to mask it from
>>   error_prepend() and such.  These crash on @error_warn, as pointed
>>   out by Akihiko Odaki.
>
> For this problem, Error can tell that it is a special destination by leaving msg NULL, for example. ERRP_GUARD() then rewrites errp it is not &error_abort and msg is NULL.

As I wrote, the defects are all fixable.  However, there has been so
little use of &error_warn, and so much of it has been unclean or
otherwise undesirable.  It's obviously prone to misuse.  I think we're
better off without it.

See also the memo "Abuse of warnings for unhandled errors and
programming errors" I posted yesterday.

> Special destinations can also have a function pointer void (*)(Error*), which will be called by error_handle(). This way, we can ensure that having a special destination will not require changes to the common code.
>
> By the way, I also asked for a comment with the migration patch series. Please reply the following if you have anything to say:
> https://lore.kernel.org/qemu-devel/9c552525-72fa-4d1e-89a2-b5c0e446935a@rsg.ci.i.u-tokyo.ac.jp/
>
> There is also an additional context:
> https://lore.kernel.org/qemu-devel/aJMsRBd9-XOMRG78@armenon-kvm.bengluru.csb/

I replied there.

I'll be on vacation the next two weeks.