| Message ID | 20180607165218.9558-9-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc/spapr/s390x: machine hotplug handler cleanups | expand |
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.
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?
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.
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.
On Fri, 8 Jun 2018 09:40:04 +0200 David Hildenbrand <david@redhat.com> 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. 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
On 08.06.2018 11:03, Igor Mammedov wrote: > On Fri, 8 Jun 2018 09:40:04 +0200 > David Hildenbrand <david@redhat.com> 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. > 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: They are, but not on s390x :) But we should never life with such assumptions - calling code may change. Passing NULL results right now not in a crash, but the "return" value in case of a failure cannot be indicated. Maybe we should even change all users of NULL for errp to use &error_abort. For now I dropped these "local_err" patches and kept only the spapr cleanups. > > 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 > >
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,
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(-)