Eduardo Habkost <ehabk...@redhat.com> writes:
> On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
>> On 08/04/2019 15.45, Wei Yang wrote:
> [...]
>> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> > index 6077d27361..b11f3b15c1 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler
>> > *hotplug_dev, DeviceState *dev,
>> > }
>> >
>> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>> > + if (*errp) {
>> > + return;
>> > + }
>>
>> Not sure, but I think you can not rely on the fact that the caller set
>> *errp = NULL already... that's why it is more common to use a local_err
>> variable and error_propagate() for such cases (which is what I did in my
>> patch).
>
> *errp can't be non-NULL (otherwise functions calling error_setg()
> would crash). errp can be NULL, though, and that's why you need
> a local_err variable.
Correct. The big comment in error.h advises:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
*
* Do *not* "optimize" this to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
* because errp may be NULL!
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
> I'd love to eliminate NULL errp from our codebase, but I couldn't
> find a way to do it that is safe and simple (i.e. not letting us
> pass NULL errp by mistake and not requiring a macro wrapping
> every `&local_err` expression).
Also, I'd prefer not not deviate even more from GError.
Apropos NULL, we often pass NULL where we really ought to pass
&error_abort.
