| Message ID | 160309730758.2739814.15821922745424652642.stgit@bahia.lan |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 3) | expand |
On 10/19/20 10:48 AM, Greg Kurz wrote: > The PC_DIMM_SLOT_PROP property is defined as: > > DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot, > PC_DIMM_UNASSIGNED_SLOT), Worth adding: #define PC_DIMM_UNASSIGNED_SLOT -1 for better understanding why it is signed. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Use object_property_get_int() instead of object_property_get_uint(). > Since spapr_memory_plug() only gets called if pc_dimm_pre_plug() > succeeded, we expect to have a valid >= 0 slot number, either because > the user passed a valid slot number or because pc_dimm_get_free_slot() > picked one up for us. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 115fc52e3b06..1b173861152f 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3433,7 +3433,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error *local_err = NULL; > SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > - uint64_t size, addr, slot; > + uint64_t size, addr; > + int64_t slot; > bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); > > size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort); > @@ -3450,11 +3451,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), > &local_err); > } else { > - slot = object_property_get_uint(OBJECT(dimm), > - PC_DIMM_SLOT_PROP, &local_err); > + slot = object_property_get_int(OBJECT(dimm), > + PC_DIMM_SLOT_PROP, &local_err); > if (local_err) { > goto out_unplug; > } > + /* We should have valid slot number at this point */ > + g_assert(slot >= 0); > spapr_add_nvdimm(dev, slot, &local_err); > } > > > >
On Mon, Oct 19, 2020 at 10:48:27AM +0200, Greg Kurz wrote: > The PC_DIMM_SLOT_PROP property is defined as: > > DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot, > PC_DIMM_UNASSIGNED_SLOT), > > Use object_property_get_int() instead of object_property_get_uint(). > Since spapr_memory_plug() only gets called if pc_dimm_pre_plug() > succeeded, we expect to have a valid >= 0 slot number, either because > the user passed a valid slot number or because pc_dimm_get_free_slot() > picked one up for us. > > Signed-off-by: Greg Kurz <groug@kaod.org> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 115fc52e3b06..1b173861152f 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3433,7 +3433,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error *local_err = NULL; > SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > - uint64_t size, addr, slot; > + uint64_t size, addr; > + int64_t slot; > bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); > > size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort); > @@ -3450,11 +3451,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), > &local_err); > } else { > - slot = object_property_get_uint(OBJECT(dimm), > - PC_DIMM_SLOT_PROP, &local_err); > + slot = object_property_get_int(OBJECT(dimm), > + PC_DIMM_SLOT_PROP, &local_err); > if (local_err) { > goto out_unplug; > } > + /* We should have valid slot number at this point */ > + g_assert(slot >= 0); > spapr_add_nvdimm(dev, slot, &local_err); > } > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 115fc52e3b06..1b173861152f 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3433,7 +3433,8 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error *local_err = NULL; SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); - uint64_t size, addr, slot; + uint64_t size, addr; + int64_t slot; bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); size = memory_device_get_region_size(MEMORY_DEVICE(dev), &error_abort); @@ -3450,11 +3451,13 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), &local_err); } else { - slot = object_property_get_uint(OBJECT(dimm), - PC_DIMM_SLOT_PROP, &local_err); + slot = object_property_get_int(OBJECT(dimm), + PC_DIMM_SLOT_PROP, &local_err); if (local_err) { goto out_unplug; } + /* We should have valid slot number at this point */ + g_assert(slot >= 0); spapr_add_nvdimm(dev, slot, &local_err); }
The PC_DIMM_SLOT_PROP property is defined as: DEFINE_PROP_INT32(PC_DIMM_SLOT_PROP, PCDIMMDevice, slot, PC_DIMM_UNASSIGNED_SLOT), Use object_property_get_int() instead of object_property_get_uint(). Since spapr_memory_plug() only gets called if pc_dimm_pre_plug() succeeded, we expect to have a valid >= 0 slot number, either because the user passed a valid slot number or because pc_dimm_get_free_slot() picked one up for us. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)