On Mon, 3 Jul 2017 14:03:07 -0300 Daniel Henrique Barboza <danie...@linux.vnet.ibm.com> wrote:
> On 07/03/2017 11:54 AM, Greg Kurz wrote: > > QEMU shouldn't abort if spapr_add_lmbs()->spapr_drc_attach() fails. > > Let's propagate the error instead, like it is done everywhere else > > where spapr_drc_attach() is called. > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > --- > > Changes in v2: - added rollback code > > --- > > hw/ppc/spapr.c | 26 ++++++++++++++++++++++---- > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 70b3fd374e2b..ba8f57a5a054 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2601,6 +2601,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t > > addr_start, uint64_t size, > > int i, fdt_offset, fdt_size; > > void *fdt; > > uint64_t addr = addr_start; > > + Error *local_err = NULL; > > > > for (i = 0; i < nr_lmbs; i++) { > > drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, > > @@ -2611,7 +2612,18 @@ static void spapr_add_lmbs(DeviceState *dev, > > uint64_t addr_start, uint64_t size, > > fdt_offset = spapr_populate_memory_node(fdt, node, addr, > > SPAPR_MEMORY_BLOCK_SIZE); > > > > - spapr_drc_attach(drc, dev, fdt, fdt_offset, errp); > > + spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > + if (local_err) { > > + while (addr > addr_start) { > > + addr -= SPAPR_MEMORY_BLOCK_SIZE; > > + drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, > > + addr / SPAPR_MEMORY_BLOCK_SIZE); > > + spapr_drc_detach(drc, dev, NULL); > > Question: why pass NULL instead of '&local_err' in detach() ? local_err already contains the error returned by spapr_drc_attach(), it cannot be used anymore. > Can we ignore any > any error that happens in detach() because you're propagating the > local_err set > in spapr_drc_attach() already? > This is a rollback path: the best we can do is to try to undo the already attached DRCs... > ps: I am aware that ATM detach() does not propagate anything to the errp > being > passed. I don't think it's wise to write new code based on this > assumption though. > ... and indeed detach() cannot fail with the current code base. :) David was even planning to drop the errp argument in the "spapr: Refactor spapr_drc_detach()" patch. Otherwise, maybe pass &error_abort since failing to rollback is likely to be a bug ? Cheers, -- Greg > > Thanks, > > > Daniel > > > + } > > + g_free(fdt); > > + error_propagate(errp, local_err); > > + return; > > + } > > addr += SPAPR_MEMORY_BLOCK_SIZE; > > } > > /* send hotplug notification to the > > @@ -2651,14 +2663,20 @@ static void spapr_memory_plug(HotplugHandler > > *hotplug_dev, DeviceState *dev, > > addr = object_property_get_uint(OBJECT(dimm), > > PC_DIMM_ADDR_PROP, &local_err); > > if (local_err) { > > - pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr); > > - goto out; > > + goto out_unplug; > > } > > > > spapr_add_lmbs(dev, addr, size, node, > > spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), > > - &error_abort); > > + &local_err); > > + if (local_err) { > > + goto out_unplug; > > + } > > + > > + return; > > > > +out_unplug: > > + pc_dimm_memory_unplug(dev, &ms->hotplug_memory, mr); > > out: > > error_propagate(errp, local_err); > > } > > > > >
pgpbDvb0D64jU.pgp
Description: OpenPGP digital signature