| Message ID | 160309727218.2739814.14722724927730985344.stgit@bahia.lan |
|---|---|
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 3) | expand |
On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote: > Hi, > > This is a followup to a previous cleanup for the sPAPR code: > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html > > The last two patches had to be dropped because they were wrongly assuming > that object_property_get_uint() returning zero meant failure. This led to > a discussion in which arose a consensus that most of the time (not to say > always) object property getters should never fail actually, ie. failure > is very likely the result of a programming error and QEMU should abort. > > This series aims at demonstrating a revelant case I've found while auditing > object property getters (this is patch 4 that I've isolated from a huge > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit > from PC DIMMs. They expect to get some properties from the DIMM object, > which happens to be set by default at the PC DIMM class level. It thus > doesn't make sense to pass an error object and propagate it when getting > them since this would lure the user into thinking they did something wrong. > > Some preliminary cleanup is done on the way, especially dropping an unused > @errp argument of pc_dimm_plug(). This affects several platforms other than > sPAPR but I guess the patch is trivial enough to go through David's tree > if it gets acks from the relevant maintainers. Since this series mostly affects ppc, I've applied it to ppc-for-5.2. It would be nice to have an acked-by from Igor or Michael for the first patch, though. > > --- > > Greg Kurz (5): > pc-dimm: Drop @errp argument of pc_dimm_plug() > spapr: Use appropriate getter for PC_DIMM_ADDR_PROP > spapr: Use appropriate getter for PC_DIMM_SLOT_PROP > spapr: Pass &error_abort when getting some PC DIMM properties > spapr: Simplify error handling in spapr_memory_plug() > > > hw/arm/virt.c | 9 +------- > hw/i386/pc.c | 8 +------ > hw/mem/pc-dimm.c | 2 +- > hw/ppc/spapr.c | 48 +++++++++++++++-------------------------- > hw/ppc/spapr_nvdimm.c | 5 +++- > include/hw/mem/pc-dimm.h | 2 +- > include/hw/ppc/spapr_nvdimm.h | 2 +- > 7 files changed, 25 insertions(+), 51 deletions(-) > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, 22 Oct 2020 15:11:42 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote: > > Hi, > > > > This is a followup to a previous cleanup for the sPAPR code: > > > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html > > > > The last two patches had to be dropped because they were wrongly assuming > > that object_property_get_uint() returning zero meant failure. This led to > > a discussion in which arose a consensus that most of the time (not to say > > always) object property getters should never fail actually, ie. failure > > is very likely the result of a programming error and QEMU should abort. > > > > This series aims at demonstrating a revelant case I've found while auditing > > object property getters (this is patch 4 that I've isolated from a huge > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit > > from PC DIMMs. They expect to get some properties from the DIMM object, > > which happens to be set by default at the PC DIMM class level. It thus > > doesn't make sense to pass an error object and propagate it when getting > > them since this would lure the user into thinking they did something wrong. > > > > Some preliminary cleanup is done on the way, especially dropping an unused > > @errp argument of pc_dimm_plug(). This affects several platforms other than > > sPAPR but I guess the patch is trivial enough to go through David's tree > > if it gets acks from the relevant maintainers. > > Since this series mostly affects ppc, I've applied it to ppc-for-5.2. > > It would be nice to have an acked-by from Igor or Michael for the > first patch, though. > David, Igor sent a R-b for patches 1 and 4. He also suggested to call spapr_drc_attach() at pre-plug time. I'll look into this, so maybe you can drop patch 5 from ppc-for-5.2 (or the entire series at your convenience). Cheers, -- Greg > > > > --- > > > > Greg Kurz (5): > > pc-dimm: Drop @errp argument of pc_dimm_plug() > > spapr: Use appropriate getter for PC_DIMM_ADDR_PROP > > spapr: Use appropriate getter for PC_DIMM_SLOT_PROP > > spapr: Pass &error_abort when getting some PC DIMM properties > > spapr: Simplify error handling in spapr_memory_plug() > > > > > > hw/arm/virt.c | 9 +------- > > hw/i386/pc.c | 8 +------ > > hw/mem/pc-dimm.c | 2 +- > > hw/ppc/spapr.c | 48 +++++++++++++++-------------------------- > > hw/ppc/spapr_nvdimm.c | 5 +++- > > include/hw/mem/pc-dimm.h | 2 +- > > include/hw/ppc/spapr_nvdimm.h | 2 +- > > 7 files changed, 25 insertions(+), 51 deletions(-) > > >
On Sun, 25 Oct 2020 11:13:40 +0100 Greg Kurz <groug@kaod.org> wrote: > On Thu, 22 Oct 2020 15:11:42 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote: > > > Hi, > > > > > > This is a followup to a previous cleanup for the sPAPR code: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html > > > > > > The last two patches had to be dropped because they were wrongly assuming > > > that object_property_get_uint() returning zero meant failure. This led to > > > a discussion in which arose a consensus that most of the time (not to say > > > always) object property getters should never fail actually, ie. failure > > > is very likely the result of a programming error and QEMU should abort. > > > > > > This series aims at demonstrating a revelant case I've found while auditing > > > object property getters (this is patch 4 that I've isolated from a huge > > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code > > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit > > > from PC DIMMs. They expect to get some properties from the DIMM object, > > > which happens to be set by default at the PC DIMM class level. It thus > > > doesn't make sense to pass an error object and propagate it when getting > > > them since this would lure the user into thinking they did something wrong. > > > > > > Some preliminary cleanup is done on the way, especially dropping an unused > > > @errp argument of pc_dimm_plug(). This affects several platforms other than > > > sPAPR but I guess the patch is trivial enough to go through David's tree > > > if it gets acks from the relevant maintainers. > > > > Since this series mostly affects ppc, I've applied it to ppc-for-5.2. > > > > It would be nice to have an acked-by from Igor or Michael for the > > first patch, though. > > > > David, > > Igor sent a R-b for patches 1 and 4. He also suggested to call > spapr_drc_attach() at pre-plug time. I'll look into this, so maybe > you can drop patch 5 from ppc-for-5.2 (or the entire series at > your convenience). > It seems that spapr_drc_attach() cannot be called at pre-plug time actually because there is no way to call spapr_drc_detach() if the device fails to realize. I think you there's nothing else to do for this series than adding Igor's r-b to patches 1 and 4. > Cheers, > > -- > Greg > > > > > > > --- > > > > > > Greg Kurz (5): > > > pc-dimm: Drop @errp argument of pc_dimm_plug() > > > spapr: Use appropriate getter for PC_DIMM_ADDR_PROP > > > spapr: Use appropriate getter for PC_DIMM_SLOT_PROP > > > spapr: Pass &error_abort when getting some PC DIMM properties > > > spapr: Simplify error handling in spapr_memory_plug() > > > > > > > > > hw/arm/virt.c | 9 +------- > > > hw/i386/pc.c | 8 +------ > > > hw/mem/pc-dimm.c | 2 +- > > > hw/ppc/spapr.c | 48 +++++++++++++++-------------------------- > > > hw/ppc/spapr_nvdimm.c | 5 +++- > > > include/hw/mem/pc-dimm.h | 2 +- > > > include/hw/ppc/spapr_nvdimm.h | 2 +- > > > 7 files changed, 25 insertions(+), 51 deletions(-) > > > > > >
On Sun, Oct 25, 2020 at 10:33:06PM +0100, Greg Kurz wrote: > On Sun, 25 Oct 2020 11:13:40 +0100 > Greg Kurz <groug@kaod.org> wrote: > > > On Thu, 22 Oct 2020 15:11:42 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > On Mon, Oct 19, 2020 at 10:47:52AM +0200, Greg Kurz wrote: > > > > Hi, > > > > > > > > This is a followup to a previous cleanup for the sPAPR code: > > > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html > > > > > > > > The last two patches had to be dropped because they were wrongly assuming > > > > that object_property_get_uint() returning zero meant failure. This led to > > > > a discussion in which arose a consensus that most of the time (not to say > > > > always) object property getters should never fail actually, ie. failure > > > > is very likely the result of a programming error and QEMU should abort. > > > > > > > > This series aims at demonstrating a revelant case I've found while auditing > > > > object property getters (this is patch 4 that I've isolated from a huge > > > > 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code > > > > is tailored to support either regular PC DIMMs or NVDIMMs, which inherit > > > > from PC DIMMs. They expect to get some properties from the DIMM object, > > > > which happens to be set by default at the PC DIMM class level. It thus > > > > doesn't make sense to pass an error object and propagate it when getting > > > > them since this would lure the user into thinking they did something wrong. > > > > > > > > Some preliminary cleanup is done on the way, especially dropping an unused > > > > @errp argument of pc_dimm_plug(). This affects several platforms other than > > > > sPAPR but I guess the patch is trivial enough to go through David's tree > > > > if it gets acks from the relevant maintainers. > > > > > > Since this series mostly affects ppc, I've applied it to ppc-for-5.2. > > > > > > It would be nice to have an acked-by from Igor or Michael for the > > > first patch, though. > > > > > > > David, > > > > Igor sent a R-b for patches 1 and 4. He also suggested to call > > spapr_drc_attach() at pre-plug time. I'll look into this, so maybe > > you can drop patch 5 from ppc-for-5.2 (or the entire series at > > your convenience). > > > > It seems that spapr_drc_attach() cannot be called at pre-plug time > actually because there is no way to call spapr_drc_detach() if > the device fails to realize. I think you there's nothing else to do > for this series than adding Igor's r-b to patches 1 and 4. Ok. In fact I'd already added Igor's r-b, just hadn't pushed out my latest tree. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Hi, This is a followup to a previous cleanup for the sPAPR code: https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg04860.html The last two patches had to be dropped because they were wrongly assuming that object_property_get_uint() returning zero meant failure. This led to a discussion in which arose a consensus that most of the time (not to say always) object property getters should never fail actually, ie. failure is very likely the result of a programming error and QEMU should abort. This series aims at demonstrating a revelant case I've found while auditing object property getters (this is patch 4 that I've isolated from a huge 50-patch series I haven't dared to post yet). The sPAPR memory hotplug code is tailored to support either regular PC DIMMs or NVDIMMs, which inherit from PC DIMMs. They expect to get some properties from the DIMM object, which happens to be set by default at the PC DIMM class level. It thus doesn't make sense to pass an error object and propagate it when getting them since this would lure the user into thinking they did something wrong. Some preliminary cleanup is done on the way, especially dropping an unused @errp argument of pc_dimm_plug(). This affects several platforms other than sPAPR but I guess the patch is trivial enough to go through David's tree if it gets acks from the relevant maintainers. --- Greg Kurz (5): pc-dimm: Drop @errp argument of pc_dimm_plug() spapr: Use appropriate getter for PC_DIMM_ADDR_PROP spapr: Use appropriate getter for PC_DIMM_SLOT_PROP spapr: Pass &error_abort when getting some PC DIMM properties spapr: Simplify error handling in spapr_memory_plug() hw/arm/virt.c | 9 +------- hw/i386/pc.c | 8 +------ hw/mem/pc-dimm.c | 2 +- hw/ppc/spapr.c | 48 +++++++++++++++-------------------------- hw/ppc/spapr_nvdimm.c | 5 +++- include/hw/mem/pc-dimm.h | 2 +- include/hw/ppc/spapr_nvdimm.h | 2 +- 7 files changed, 25 insertions(+), 51 deletions(-) -- Greg