Igor Mammedov <imamm...@redhat.com> writes: > On Thu, 08 Jul 2021 15:08:57 +0200 > Markus Armbruster <arm...@redhat.com> wrote: > >> Daniel Henrique Barboza <danielhb...@gmail.com> writes: >> >> > MEM_UNPLUG_ERROR is deprecated since the introduction of >> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of >> > MEM_UNPLUG_ERROR is pending. >> > >> > CC: Michael S. Tsirkin <m...@redhat.com> >> > CC: Igor Mammedov <imamm...@redhat.com> >> > Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> >> > Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com> >> > --- >> > hw/acpi/memory_hotplug.c | 13 +++++++++++-- >> > 1 file changed, 11 insertions(+), 2 deletions(-) >> > >> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> > index af37889423..fb9f4d2de7 100644 >> > --- a/hw/acpi/memory_hotplug.c >> > +++ b/hw/acpi/memory_hotplug.c >> > @@ -8,6 +8,7 @@ >> > #include "qapi/error.h" >> > #include "qapi/qapi-events-acpi.h" >> > #include "qapi/qapi-events-machine.h" >> > +#include "qapi/qapi-events-qdev.h" >> > >> > #define MEMORY_SLOTS_NUMBER "MDNR" >> > #define MEMORY_HOTPLUG_IO_REGION "HPMR" >> > @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, >> > hwaddr addr, uint64_t data, >> > /* call pc-dimm unplug cb */ >> > hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); >> > if (local_err) { >> > + const char *error_pretty = error_get_pretty(local_err); >> > + >> > trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector); >> > - qapi_event_send_mem_unplug_error(dev->id, >> > - >> > error_get_pretty(local_err)); >> > + >> > + /* >> > + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR >> > + * while the deprecation of MEM_UNPLUG_ERROR is >> > + * pending. >> > + */ >> > + qapi_event_send_mem_unplug_error(dev->id, error_pretty); >> > + qapi_event_send_device_unplug_error(dev->id, >> > error_pretty); >> > error_free(local_err); >> > break; >> > } >> >> Same question as for PATCH 2: can dev->id be null? > only theoretically (if memory device were created directly without > using device_add), which as far as I know is not the case as all > memory devices are created using -device/device_add so far. > > ( for device_add case see qdev_device_add->qdev_set_id where > 'id' is set to user provided or to generated "device[%d]" value)
Something is set to a generated value, but it's not dev->id :) void qdev_set_id(DeviceState *dev, const char *id) @id is the value of id=... It may be null. dev->id still is null here. { if (id) { dev->id = id; } dev->id is now the value of id=... It may be null. if (dev->id) { object_property_add_child(qdev_get_peripheral(), dev->id, OBJECT(dev)); If the user specified id=..., add @dev as child of /peripheral. The child's name is the (non-null) value of id=... } else { static int anon_count; gchar *name = g_strdup_printf("device[%d]", anon_count++); object_property_add_child(qdev_get_peripheral_anon(), name, OBJECT(dev)); g_free(name); Else, add @dev as child of /peripheral-anon. The child's name is made up. } } dev->id is still the value of id=..., i.e. it may be null. Sure dereferencing dev->id in acpi_memory_hotplug_write() is safe?