On Fri, 8 Jun 2018 10:41:36 +0200 David Hildenbrand <da...@redhat.com> wrote:
> On 08.06.2018 10:39, Igor Mammedov wrote: > > On Fri, 8 Jun 2018 10:07:31 +0200 > > Thomas Huth <th...@redhat.com> 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 <da...@redhat.com> wrote: > >>>> > >>>>> On 08.06.2018 09:34, Greg Kurz wrote: > >>>>>> On Thu, 7 Jun 2018 18:52:12 +0200 > >>>>>> David Hildenbrand <da...@redhat.com> wrote: > >>>>>> > >>>>>>> The node property can always be queried and the value has already been > >>>>>>> verified in pc_dimm_realize(). > >>>>>>> > >>>>>>> Signed-off-by: David Hildenbrand <da...@redhat.com> > >>>>>>> --- > >>>>>>> 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) Add it to wiki page for bite sized tasks? > > > > >> > >> Thomas > > > >