Re: [PATCH 4/5] spapr: Pass &error_abort when getting some PC DIMM properties

[Igor Mammedov]

On Mon, 19 Oct 2020 10:48:41 +0200
Greg Kurz <groug@kaod.org> wrote:

> Both PC_DIMM_SLOT_PROP and PC_DIMM_ADDR_PROP are defined in the
> default property list of the PC DIMM device class:
> 
>     DEFINE_PROP_UINT64(PC_DIMM_ADDR_PROP, PCDIMMDevice, addr, 0),
> 
>     DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot,
>                       PC_DIMM_UNASSIGNED_SLOT),
> 
> They should thus be always gettable for both PC DIMMs and NVDIMMs.
> An error in getting them can only be the result of a programming
> error. It doesn't make much sense to propagate the error in this
> case. Abort instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

TODO for future,
get rid of local_err in spapr_memory_plug() altogether, it should not fail.
it needs moving check from spapr_drc_attach() to spapr_memory_pre_plug() time.

that will clear up (a bit) road for dropping errp in spapr_memory_plug()
> ---
>  hw/ppc/spapr.c |   17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1b173861152f..62f217a6b914 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3443,19 +3443,13 @@ static void spapr_memory_plug(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  
>      if (!is_nvdimm) {
>          addr = object_property_get_uint(OBJECT(dimm),
> -                                        PC_DIMM_ADDR_PROP, &local_err);
> -        if (local_err) {
> -            goto out_unplug;
> -        }
> +                                        PC_DIMM_ADDR_PROP, &error_abort);
>          spapr_add_lmbs(dev, addr, size,
>                         spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT),
>                         &local_err);
>      } else {
>          slot = object_property_get_int(OBJECT(dimm),
> -                                       PC_DIMM_SLOT_PROP, &local_err);
> -        if (local_err) {
> -            goto out_unplug;
> -        }
> +                                       PC_DIMM_SLOT_PROP, &error_abort);
>          /* We should have valid slot number at this point */
>          g_assert(slot >= 0);
>          spapr_add_nvdimm(dev, slot, &local_err);
> @@ -3634,7 +3628,6 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
>                                          DeviceState *dev, Error **errp)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> -    Error *local_err = NULL;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      uint32_t nr_lmbs;
>      uint64_t size, addr_start, addr;
> @@ -3650,11 +3643,7 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
>      nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
>  
>      addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
> -                                         &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +                                          &error_abort);
>  
>      /*
>       * An existing pending dimm state for this DIMM means that there is an
> 
> 
>