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

[Igor Mammedov]

On Fri, 8 Jun 2018 09:40:04 +0200
David Hildenbrand <address@hidden> 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 <address@hidden> wrote:
> >>  
> >>> Let's introduce and use local error variables in the hotplug handler
> >>> functions.
> >>>
> >>> Signed-off-by: David Hildenbrand <address@hidden>
> >>> ---
> >>>  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.
hotplug handlers aren't called with NULL errp, so it not really necessary.
To be on the safe side we can ensure that errp is not NULL doing something
like:

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986..dc9e4bf 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -52,6 +52,7 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
 {
     HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
 
+    g_assert(errp);
     if (hdc->unplug) {
         hdc->unplug(plug_handler, plugged_dev, errp);
     }

and do it for all similar wrappers in this file