Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types

[Markus Armbruster]

Eduardo Habkost <address@hidden> 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.