Akihiko Odaki <od...@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-3e81a1d41...@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-b5c0e4469...@rsg.ci.i.u-tokyo.ac.jp/ > > There is also an additional context: > https://lore.kernel.org/qemu-devel/ajmsrbd9-xomr...@armenon-kvm.bengluru.csb/ I replied there. I'll be on vacation the next two weeks.
