Hi, On 11/04/2019 03.56, Wei Yang wrote: > On Mon, Apr 08, 2019 at 09:29:11PM +0000, Wei Yang wrote: >>>> >>>> Thomas, >>>> >>>> Thanks for pointing this out, while I have some different idea on how to >>>> fix >>>> this. >>>> >>>> The reason of the core dump is errp already been set in >>>> hotplug_handler_pre_plug(), and this function check acpi hotplug >>>> capability. >>>> The order of this check is correct, while we should return when errp is >>>> set >>>> in hotplug_handler_pre_plug(). >>>> >>>> I got a fix like this, which I have tested and looks good to me. >>>> >>>> >>>> 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). >>> >> >> Ok, that's fine for me. >> >>> Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in >>> '-M'" check to be done first? >>> >> >> Because this function pc_memory_pre_plug() will be called not only when >> nvdimm is hot-plugged but also dimm is hot-plugged. And >> hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug >> capability. >> >> So the check in pc_memory_pre_plug() is from generic to specific: >> 1. Do we have capability to hot-plug? >> 2. If the device is nvdimm, do we enabled nvdimm? >> > > Thomas > > Do you think this is a reasonable explanation?
Fine for me, I don't mind either way as long as the crash is fixed. Feel free to send a patch that restores the desired order. Thomas