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. > > Thomas