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 <da...@redhat.com> wrote:
>>>
>>>> Let's introduce and use local error variables in the hotplug handler
>>>> functions.
>>>>
>>>> Signed-off-by: David Hildenbrand <da...@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.
