Re: [Qemu-ppc] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[Igor Mammedov]

On Fri, 8 Jun 2018 10:07:31 +0200
Thomas Huth <address@hidden> 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 <address@hidden> wrote:
> >>  
> >>> On 08.06.2018 09:34, Greg Kurz wrote:  
> >>>> On Thu,  7 Jun 2018 18:52:12 +0200
> >>>> David Hildenbrand <address@hidden> wrote:
> >>>>     
> >>>>> The node property can always be queried and the value has already been
> >>>>> verified in pc_dimm_realize().
> >>>>>
> >>>>> Signed-off-by: David Hildenbrand <address@hidden>
> >>>>> ---
> >>>>>  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