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

[David Hildenbrand]

On 08.06.2018 10:39, Igor Mammedov wrote:
> 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.

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