| Message ID | 20210719200827.1507276-2-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | DEVICE_UNPLUG_ERROR QAPI event | expand |
I apologize for the not reviewing this promptly. Daniel Henrique Barboza <danielhb413@gmail.com> writes: > qapi_event_send_mem_unplug_error() deals with @device being NULL by > replacing it with an empty string ("") when emitting the event. Aside > from the fact that this is a side effect that can be patched someday, I guess by "side effect" you allude to the output visitor's misfeature to map null pointer to "". > there's also the lack of utility that the event brings to listeners, > e.g. "a memory unplug error happened somewhere". True. > We're better of not emitting the event if dev->id is NULL. On a green field, yes. But is it worth an incompatible change now? I doubt it. > Next patches > will introduce a new device unplug error event that is better suited to > deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be > emitted to avoid breaking existing APIs, but it'll be deprecated and > removed in the future. > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/acpi/memory_hotplug.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index af37889423..e37acb0367 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -177,9 +177,14 @@ 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)); > + > + if (dev->id) { > + qapi_event_send_mem_unplug_error(dev->id, error_pretty); > + } > + > error_free(local_err); > break; > } Three options: 1. Make the incompatible change. Keep this patch. Needs a release note. 2. Continue to rely on the output visitor's misfeature. Drop this patch. Relying on the misfeature is best avoided, at least in new code. 3. Make the mapping explicit here, and avoid relying on the misfeature. Something like the appended patch. I don't like 1. I think the choice between 2. and 3. depends on how it fits with the remainder of this series. diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index af37889423..89c411dd5c 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -178,7 +178,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); if (local_err) { trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector); - qapi_event_send_mem_unplug_error(dev->id, + qapi_event_send_mem_unplug_error(dev->id ?: "", error_get_pretty(local_err)); error_free(local_err); break;
On 8/7/21 10:38 AM, Markus Armbruster wrote: > I apologize for the not reviewing this promptly. > > Daniel Henrique Barboza <danielhb413@gmail.com> writes: > >> qapi_event_send_mem_unplug_error() deals with @device being NULL by >> replacing it with an empty string ("") when emitting the event. Aside >> from the fact that this is a side effect that can be patched someday, > > I guess by "side effect" you allude to the output visitor's misfeature > to map null pointer to "". > >> there's also the lack of utility that the event brings to listeners, >> e.g. "a memory unplug error happened somewhere". > > True. > >> We're better of not emitting the event if dev->id is NULL. > > On a green field, yes. But is it worth an incompatible change now? I > doubt it. > >> Next patches >> will introduce a new device unplug error event that is better suited to >> deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be >> emitted to avoid breaking existing APIs, but it'll be deprecated and >> removed in the future. >> >> Suggested-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Greg Kurz <groug@kaod.org> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/acpi/memory_hotplug.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> index af37889423..e37acb0367 100644 >> --- a/hw/acpi/memory_hotplug.c >> +++ b/hw/acpi/memory_hotplug.c >> @@ -177,9 +177,14 @@ 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)); >> + >> + if (dev->id) { >> + qapi_event_send_mem_unplug_error(dev->id, error_pretty); >> + } >> + >> error_free(local_err); >> break; >> } > > Three options: > > 1. Make the incompatible change. Keep this patch. Needs a release > note. > > 2. Continue to rely on the output visitor's misfeature. Drop this > patch. > > Relying on the misfeature is best avoided, at least in new code. > > 3. Make the mapping explicit here, and avoid relying on the misfeature. > Something like the appended patch. I like (3) more. If no one opposes I'll respin patches 1 and 2 with this approach. Daniel > > I don't like 1. I think the choice between 2. and 3. depends on how it > fits with the remainder of this series. > > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index af37889423..89c411dd5c 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -178,7 +178,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, > hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); > if (local_err) { > trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector); > - qapi_event_send_mem_unplug_error(dev->id, > + qapi_event_send_mem_unplug_error(dev->id ?: "", > error_get_pretty(local_err)); > error_free(local_err); > break; >
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index af37889423..e37acb0367 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -177,9 +177,14 @@ 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)); + + if (dev->id) { + qapi_event_send_mem_unplug_error(dev->id, error_pretty); + } + error_free(local_err); break; }
qapi_event_send_mem_unplug_error() deals with @device being NULL by replacing it with an empty string ("") when emitting the event. Aside from the fact that this is a side effect that can be patched someday, there's also the lack of utility that the event brings to listeners, e.g. "a memory unplug error happened somewhere". We're better of not emitting the event if dev->id is NULL. Next patches will introduce a new device unplug error event that is better suited to deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be emitted to avoid breaking existing APIs, but it'll be deprecated and removed in the future. Suggested-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Greg Kurz <groug@kaod.org> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/acpi/memory_hotplug.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)