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? -- Wei Yang Help you, Help me