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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 8 Jun 2018 10:41:36 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 08.06.2018 10:39, Igor Mammedov wrote:
> > On Fri, 8 Jun 2018 10:07:31 +0200
> > Thomas Huth <thuth@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 <david@redhat.com> wrote:
> >>>>    
> >>>>> On 08.06.2018 09:34, Greg Kurz wrote:    
> >>>>>> On Thu,  7 Jun 2018 18:52:12 +0200
> >>>>>> David Hildenbrand <david@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 <david@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  
> >   
> 
>