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

[Thomas Huth]

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.

 Thomas