Re: [Qemu-ppc] [PATCH v2] spapr: fix memory hotplug error path

[Greg Kurz]

On Mon, 3 Jul 2017 14:03:07 -0300
Daniel Henrique Barboza <address@hidden> 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 <address@hidden>
> > ---
> > 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);
> >   }
> >
> >  
>

pgpRLOB8uaAwD.pgp
Description: OpenPGP digital signature