| Message ID | 20180607165218.9558-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc/spapr/s390x: machine hotplug handler cleanups | expand |
On Thu, Jun 07, 2018 at 06:52:12PM +0200, David Hildenbrand wrote: > The node property can always be queried and the value has already been > verified in pc_dimm_realize(). > > Signed-off-by: David Hildenbrand <david@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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) { > - return; > - } > - if (node < 0 || node >= MAX_NODES) { > - error_setg(errp, "Invaild node %d", node); > - return; > - } > + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL); > > spapr_memory_plug(hotplug_dev, dev, node, errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
On Thu, 7 Jun 2018 18:52:12 +0200 David Hildenbrand <david@redhat.com> wrote: > The node property can always be queried and the value has already been > verified in pc_dimm_realize(). > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > 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; > - } > + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL); Maybe pass &error_abort ? > > spapr_memory_plug(hotplug_dev, dev, node, errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
On 08.06.2018 09:34, Greg Kurz wrote: > On Thu, 7 Jun 2018 18:52:12 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> The node property can always be queried and the value has already been >> verified in pc_dimm_realize(). >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> 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; >> - } >> + 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. Thanks! > >> >> spapr_memory_plug(hotplug_dev, dev, node, errp); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { >
On Fri, 8 Jun 2018 09:42:48 +0200 David Hildenbrand <david@redhat.com> wrote: > On 08.06.2018 09:34, Greg Kurz wrote: > > On Thu, 7 Jun 2018 18:52:12 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> The node property can always be queried and the value has already been > >> verified in pc_dimm_realize(). > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> 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; > >> - } > >> + 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. Reviewed-by: Greg Kurz <groug@kaod.org> > Thanks! > > > > >> > >> spapr_memory_plug(hotplug_dev, dev, node, errp); > >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > > > >
On 08.06.2018 09:46, Greg Kurz wrote: > On Fri, 8 Jun 2018 09:42:48 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 08.06.2018 09:34, Greg Kurz wrote: >>> On Thu, 7 Jun 2018 18:52:12 +0200 >>> David Hildenbrand <david@redhat.com> wrote: >>> >>>> The node property can always be queried and the value has already been >>>> verified in pc_dimm_realize(). >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> 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; >>>> - } >>>> + 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 :) Thanks! > > Reviewed-by: Greg Kurz <groug@kaod.org> > >> Thanks! >> >>> >>>> >>>> spapr_memory_plug(hotplug_dev, dev, node, errp); >>>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { >>> >> >> >
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 <david@redhat.com> wrote: >> >>> On 08.06.2018 09:34, Greg Kurz wrote: >>>> On Thu, 7 Jun 2018 18:52:12 +0200 >>>> David Hildenbrand <david@redhat.com> wrote: >>>> >>>>> The node property can always be queried and the value has already been >>>>> verified in pc_dimm_realize(). >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> 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
On Fri, Jun 08, 2018 at 09:46:57AM +0200, Greg Kurz wrote: > On Fri, 8 Jun 2018 09:42:48 +0200 > David Hildenbrand <david@redhat.com> wrote: > > > On 08.06.2018 09:34, Greg Kurz wrote: > > > On Thu, 7 Jun 2018 18:52:12 +0200 > > > David Hildenbrand <david@redhat.com> wrote: > > > > > >> The node property can always be queried and the value has already been > > >> verified in pc_dimm_realize(). > > >> > > >> Signed-off-by: David Hildenbrand <david@redhat.com> > > >> --- > > >> 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; > > >> - } > > >> + 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. Actually, I missed this - error_abort is preferable. That's general the right choice for things that shouldn't ever fail. This way if they *do* fail we get a clear error immediately. > Reviewed-by: Greg Kurz <groug@kaod.org>
On 08.06.2018 10:20, David Gibson wrote: > On Fri, Jun 08, 2018 at 09:46:57AM +0200, Greg Kurz wrote: >> On Fri, 8 Jun 2018 09:42:48 +0200 >> David Hildenbrand <david@redhat.com> wrote: >> >>> On 08.06.2018 09:34, Greg Kurz wrote: >>>> On Thu, 7 Jun 2018 18:52:12 +0200 >>>> David Hildenbrand <david@redhat.com> wrote: >>>> >>>>> The node property can always be queried and the value has already been >>>>> verified in pc_dimm_realize(). >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> 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; >>>>> - } >>>>> + 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. > > Actually, I missed this - error_abort is preferable. That's general > the right choice for things that shouldn't ever fail. This way if > they *do* fail we get a clear error immediately. error_abort it is :) > >> Reviewed-by: Greg Kurz <groug@kaod.org> > >
On Fri, 8 Jun 2018 10:07:31 +0200 Thomas Huth <thuth@redhat.com> wrote: > 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 <david@redhat.com> wrote: > >> > >>> On 08.06.2018 09:34, Greg Kurz wrote: > >>>> On Thu, 7 Jun 2018 18:52:12 +0200 > >>>> David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>>> The node property can always be queried and the value has already been > >>>>> verified in pc_dimm_realize(). > >>>>> > >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>>> --- > >>>>> 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. +1 for error_abort call shouldn't fail, but if does it won't be silently ignored and introduce undefined behavior. > > Thomas
On 08.06.2018 10:39, Igor Mammedov wrote: > On Fri, 8 Jun 2018 10:07:31 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> 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 <david@redhat.com> wrote: >>>> >>>>> On 08.06.2018 09:34, Greg Kurz wrote: >>>>>> On Thu, 7 Jun 2018 18:52:12 +0200 >>>>>> David Hildenbrand <david@redhat.com> wrote: >>>>>> >>>>>>> The node property can always be queried and the value has already been >>>>>>> verified in pc_dimm_realize(). >>>>>>> >>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>>> --- >>>>>>> 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. > +1 for error_abort > call shouldn't fail, but if does it won't be silently ignored > and introduce undefined behavior. Maybe we should fix the others that pass in NULL. (no, not me :D - I'm already busy with your requested pre_plug handling) > >> >> Thomas >
On Fri, 8 Jun 2018 10:41:36 +0200 David Hildenbrand <david@redhat.com> wrote: > On 08.06.2018 10:39, Igor Mammedov wrote: > > On Fri, 8 Jun 2018 10:07:31 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> 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 <david@redhat.com> wrote: > >>>> > >>>>> On 08.06.2018 09:34, Greg Kurz wrote: > >>>>>> On Thu, 7 Jun 2018 18:52:12 +0200 > >>>>>> David Hildenbrand <david@redhat.com> wrote: > >>>>>> > >>>>>>> The node property can always be queried and the value has already been > >>>>>>> verified in pc_dimm_realize(). > >>>>>>> > >>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>>>>> --- > >>>>>>> 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. > > +1 for error_abort > > call shouldn't fail, but if does it won't be silently ignored > > and introduce undefined behavior. > > Maybe we should fix the others that pass in NULL. > > (no, not me :D - I'm already busy with your requested pre_plug handling) Add it to wiki page for bite sized tasks? > > > > >> > >> Thomas > > > >
>>>> +1 for error_abort, even if it takes another line. >>> +1 for error_abort >>> call shouldn't fail, but if does it won't be silently ignored >>> and introduce undefined behavior. >> >> Maybe we should fix the others that pass in NULL. >> >> (no, not me :D - I'm already busy with your requested pre_plug handling) > Add it to wiki page for bite sized tasks? Done.
On Fri, 8 Jun 2018 11:24:51 +0200 David Hildenbrand <david@redhat.com> wrote: > >>>> +1 for error_abort, even if it takes another line. > >>> +1 for error_abort > >>> call shouldn't fail, but if does it won't be silently ignored > >>> and introduce undefined behavior. > >> > >> Maybe we should fix the others that pass in NULL. > >> > >> (no, not me :D - I'm already busy with your requested pre_plug handling) > > Add it to wiki page for bite sized tasks? > > Done. > > FWIW, I've also added a line to check and possibly fix places where we do 'if (*errp)', which would cause QEMU to crash if the caller passes NULL. $ git grep 'if (\*errp)' hmp.c: if (*errp) { hw/ipmi/isa_ipmi_bt.c: if (*errp) hw/ipmi/isa_ipmi_kcs.c: if (*errp) hw/mem/memory-device.c: if (*errp) { hw/mem/memory-device.c: if (*errp) { hw/ppc/spapr.c: if (*errp) { hw/s390x/event-facility.c: if (*errp) { include/qapi/error.h: * if (*errp) { // WRONG! qga/commands-posix.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { target/s390x/cpu_models.c: if (*errp) { tests/test-crypto-tlscredsx509.c: if (*errp) { tests/test-io-channel-tls.c: if (*errp) {
On 08.06.2018 12:52, Greg Kurz wrote: > On Fri, 8 Jun 2018 11:24:51 +0200 > David Hildenbrand <david@redhat.com> wrote: > >>>>>> +1 for error_abort, even if it takes another line. >>>>> +1 for error_abort >>>>> call shouldn't fail, but if does it won't be silently ignored >>>>> and introduce undefined behavior. >>>> >>>> Maybe we should fix the others that pass in NULL. >>>> >>>> (no, not me :D - I'm already busy with your requested pre_plug handling) >>> Add it to wiki page for bite sized tasks? >> >> Done. >> >> > > FWIW, I've also added a line to check and possibly fix places where we do > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL. > > $ git grep 'if (\*errp)' > hmp.c: if (*errp) { > hw/ipmi/isa_ipmi_bt.c: if (*errp) > hw/ipmi/isa_ipmi_kcs.c: if (*errp) > hw/mem/memory-device.c: if (*errp) { > hw/mem/memory-device.c: if (*errp) { > hw/ppc/spapr.c: if (*errp) { > hw/s390x/event-facility.c: if (*errp) { > include/qapi/error.h: * if (*errp) { // WRONG! > qga/commands-posix.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > target/s390x/cpu_models.c: if (*errp) { > tests/test-crypto-tlscredsx509.c: if (*errp) { > tests/test-io-channel-tls.c: if (*errp) { > I think the more important part is actually looking out for people that use NULL instead of error_abort. This way we won't silently ignore errors.
On Fri, 8 Jun 2018 13:28:01 +0200 David Hildenbrand <david@redhat.com> wrote: > On 08.06.2018 12:52, Greg Kurz wrote: > > On Fri, 8 Jun 2018 11:24:51 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >>>>>> +1 for error_abort, even if it takes another line. > >>>>> +1 for error_abort > >>>>> call shouldn't fail, but if does it won't be silently ignored > >>>>> and introduce undefined behavior. > >>>> > >>>> Maybe we should fix the others that pass in NULL. > >>>> > >>>> (no, not me :D - I'm already busy with your requested pre_plug handling) > >>> Add it to wiki page for bite sized tasks? > >> > >> Done. > >> > >> > > > > FWIW, I've also added a line to check and possibly fix places where we do > > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL. > > > > $ git grep 'if (\*errp)' > > hmp.c: if (*errp) { > > hw/ipmi/isa_ipmi_bt.c: if (*errp) > > hw/ipmi/isa_ipmi_kcs.c: if (*errp) > > hw/mem/memory-device.c: if (*errp) { > > hw/mem/memory-device.c: if (*errp) { > > hw/ppc/spapr.c: if (*errp) { > > hw/s390x/event-facility.c: if (*errp) { > > include/qapi/error.h: * if (*errp) { // WRONG! > > qga/commands-posix.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > tests/test-crypto-tlscredsx509.c: if (*errp) { > > tests/test-io-channel-tls.c: if (*errp) { > > > > I think the more important part is actually looking out for people that > use NULL instead of error_abort. This way we won't silently ignore errors. I think we can assume that the callers here all pass in !NULL. Would probably make sense to change these anyway because (a) better safe than sorry, and (b) make sure new code does not copy it.
On Fri, 8 Jun 2018 13:28:01 +0200 David Hildenbrand <david@redhat.com> wrote: > On 08.06.2018 12:52, Greg Kurz wrote: > > On Fri, 8 Jun 2018 11:24:51 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >>>>>> +1 for error_abort, even if it takes another line. > >>>>> +1 for error_abort > >>>>> call shouldn't fail, but if does it won't be silently ignored > >>>>> and introduce undefined behavior. > >>>> > >>>> Maybe we should fix the others that pass in NULL. > >>>> > >>>> (no, not me :D - I'm already busy with your requested pre_plug handling) > >>> Add it to wiki page for bite sized tasks? > >> > >> Done. > >> > >> > > > > FWIW, I've also added a line to check and possibly fix places where we do > > 'if (*errp)', which would cause QEMU to crash if the caller passes NULL. > > > > $ git grep 'if (\*errp)' > > hmp.c: if (*errp) { > > hw/ipmi/isa_ipmi_bt.c: if (*errp) > > hw/ipmi/isa_ipmi_kcs.c: if (*errp) > > hw/mem/memory-device.c: if (*errp) { > > hw/mem/memory-device.c: if (*errp) { > > hw/ppc/spapr.c: if (*errp) { > > hw/s390x/event-facility.c: if (*errp) { > > include/qapi/error.h: * if (*errp) { // WRONG! > > qga/commands-posix.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > target/s390x/cpu_models.c: if (*errp) { > > tests/test-crypto-tlscredsx509.c: if (*errp) { > > tests/test-io-channel-tls.c: if (*errp) { > > > > I think the more important part is actually looking out for people that > use NULL instead of error_abort. This way we won't silently ignore errors. > I agree that we should probably pass &error_abort in many places, but passing NULL isn't bad per se. It means any failure in the callee is unimportant enough that we can simply ignore it. The error framework provides this possibility and so we should never dereference errp.
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) { - return; - } - if (node < 0 || node >= MAX_NODES) { - error_setg(errp, "Invaild node %d", node); - return; - } + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, NULL); spapr_memory_plug(hotplug_dev, dev, node, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
The node property can always be queried and the value has already been verified in pc_dimm_realize(). Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/ppc/spapr.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)