Re: [Qemu-devel] [PATCH v1 8/8] s390x: local error handling in hotplug handler functions

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 06/08/2018 09:40 AM, David Hildenbrand wrote:
> On 08.06.2018 09:27, Christian Borntraeger wrote:
>>
>>
>> On 06/08/2018 09:25 AM, Cornelia Huck wrote:
>>> On Thu,  7 Jun 2018 18:52:18 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> Let's introduce and use local error variables in the hotplug handler
>>>> functions.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  hw/s390x/s390-virtio-ccw.c | 11 ++++++++---
>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 7ae5fb38dd..29ea50a177 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -434,18 +434,23 @@ static void s390_machine_reset(void)
>>>>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
>>>>                                       DeviceState *dev, Error **errp)
>>>>  {
>>>> +    Error *local_err = NULL;
>>>> +
>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> -        s390_cpu_plug(hotplug_dev, dev, errp);
>>>> +        s390_cpu_plug(hotplug_dev, dev, &local_err);
>>>>      }
>>>> +    error_propagate(errp, local_err);
>>>>  }
>>>>  
>>>>  static void s390_machine_device_unplug_request(HotplugHandler *hotplug_dev,
>>>>                                                 DeviceState *dev, Error **errp)
>>>>  {
>>>> +    Error *local_err = NULL;
>>>> +
>>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> -        error_setg(errp, "CPU hot unplug not supported on this machine");
>>>> -        return;
>>>> +        error_setg(&local_err, "CPU hot unplug not supported on this machine");
>>>>      }
>>>> +    error_propagate(errp, local_err);
>>>>  }
>>>>  
>>>>  static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms,
>>>
>>> Just seeing this patch by itself, it does not really make much sense.
>>> Even if this is a split out clean-up series, I'd prefer this to go
>>> together with a patch that actually adds something more to the
>>> plug/unplug functions.
>>
>> +1. It is hard to see the "why". Maybe a better patch description could help here?
>>
> 
> When checking for an error (*errp) we should make sure that we don't
> dereference the NULL pointer. I will be doing that in the future (memory
> devices), but as you both don't seem to like this patch, I'll drop it
> for now.

With such a patch description (making the handler safe against NULL errp) the patch
suddenly makes sense on its own.