Re: [PATCH v2 2/3] hw/usb/hcd-xhci-pci: Abort if setting link property failed

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote:
>> On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote:
>> > When you know that all callers handle errors like &error_fatal does, use
>> > of &error_fatal doesn't produce wrong behavior.  It's still kind of
>> > wrong, because relying on such a non-local argument without a genuine
>> > need is.
>> 
>> Not using error_fatal results in quite a bit of extra boilerplate
>> for all those extra explicit "check for failure, print the error
>> message and exit" points.
>
> I don't get it.  There's no need for extra boilerplate if the
> caller is using &error_fatal.
>
>> If the MachineState init function took
>> an Error** that might be a mild encouragement to "pass an Error
>> upward rather than exiting"; but it doesn't.
>
> Agreed.
>
>> 
>> Right now we have nearly a thousand instances of error_fatal
>> in the codebase, incidentally.
>
> It looks like 73 of them are in functions that take an Error**
> argument.
>
> Coccinelle patch for finding them:
>
> @@
> typedef Error;
> type T;
> identifier errp, fn;
> @@
>  T fn(..., Error **errp)
>  {
>  ...
> *&error_fatal
>  ...
>  }
>
>
> Coccinelle output:

[...]

These do look suspicious to me.