On 08.06.2018 10:39, Igor Mammedov wrote: > On Fri, 8 Jun 2018 10:07:31 +0200 > Thomas Huth <[email protected]> wrote: > >> On 08.06.2018 09:48, David Hildenbrand wrote: >>> On 08.06.2018 09:46, Greg Kurz wrote: >>>> On Fri, 8 Jun 2018 09:42:48 +0200 >>>> David Hildenbrand <[email protected]> wrote: >>>> >>>>> On 08.06.2018 09:34, Greg Kurz wrote: >>>>>> On Thu, 7 Jun 2018 18:52:12 +0200 >>>>>> David Hildenbrand <[email protected]> wrote: >>>>>> >>>>>>> The node property can always be queried and the value has already been >>>>>>> verified in pc_dimm_realize(). >>>>>>> >>>>>>> Signed-off-by: David Hildenbrand <[email protected]> >>>>>>> --- >>>>>>> hw/ppc/spapr.c | 9 +-------- >>>>>>> 1 file changed, 1 insertion(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>>>>>> index 2375cbee12..d038f3243e 100644 >>>>>>> --- a/hw/ppc/spapr.c >>>>>>> +++ b/hw/ppc/spapr.c >>>>>>> @@ -3578,14 +3578,7 @@ static void >>>>>>> spapr_machine_device_plug(HotplugHandler *hotplug_dev, >>>>>>> error_setg(errp, "Memory hotplug not supported for this >>>>>>> machine"); >>>>>>> return; >>>>>>> } >>>>>>> - node = object_property_get_uint(OBJECT(dev), >>>>>>> PC_DIMM_NODE_PROP, errp); >>>>>>> - if (*errp) { >>>>>> >>>>>> Good riddance :) >>>>>> >>>>>>> - return; >>>>>>> - } >>>>>>> - if (node < 0 || node >= MAX_NODES) { >>>>>>> - error_setg(errp, "Invaild node %d", node); >>>>>>> - return; >>>>>>> - } >> >> Maybe turn that into an assert() instead? ... just for the paranoids ;-) >> >>>>>>> + node = object_property_get_uint(OBJECT(dev), >>>>>>> PC_DIMM_NODE_PROP, NULL); >>>>>> >>>>>> Maybe pass &error_abort ? >>>>> >>>>> I'm using the same access scheme as in hw/acpi/memory_hotplug.c >>>>> >>>>> ("error ignored" vs. "error leads to an abort") - but this will actually >>>>> never fail. But I can use error_abort here, does not matter. >>>>> >>>> >>>> Heh, /me paranoid but this is David's call and he acked that already >>>> so I guess it's okay. >>> >>> NULL makes it fit into a single line :) >> >> +1 for error_abort, even if it takes another line. > +1 for error_abort > call shouldn't fail, but if does it won't be silently ignored > and introduce undefined behavior.
Maybe we should fix the others that pass in NULL. (no, not me :D - I'm already busy with your requested pre_plug handling) > >> >> Thomas > -- Thanks, David / dhildenb
