Re: [PATCH v5 1/7] hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:33 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> qapi_event_send_device_unplug_error() deals with @device being NULL

s/qapi_event_send_device_unplug_error/qapi_event_send_mem_unplug_error ?

since I only see qapi_event_send_mem_unplug_error() in the diff.

> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Appart from the nit in the changelog,

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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;
>              }