On Wed, Jul 19, 2017 at 02:24:09PM +0530, Bharata B Rao wrote: > Commit 0cffce56 (hw/ppc/spapr.c: adding pending_dimm_unplugs to > sPAPRMachineState) introduced a new way to track pending LMBs of DIMM > device that is marked for removal. Since this commit we can hit the > assert in spapr_pending_dimm_unplugs_add() in the following situation: > > - DIMM device removal fails as the guest doesn't allow the removal. > - Subsequent attempt to remove the same DIMM would hit the assert > as the corresponding sPAPRDIMMState is still part of the > pending_dimm_unplugs list. > > Fix this by removing the assert and conditionally adding the > sPAPRDIMMState to pending_dimm_unplugs list only when it is not > already present. > > Fixes: 0cffce56ae3501c5783d779f97993ce478acf856 > Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com>
Sounds like a reasonable change based on the rationale above.
However, can you add a comment here explaining the situation in which
the entry already exists.
> ---
> hw/ppc/spapr.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1cb09e7..990bb2d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2853,8 +2853,9 @@ static sPAPRDIMMState
> *spapr_pending_dimm_unplugs_find(sPAPRMachineState *s,
> static void spapr_pending_dimm_unplugs_add(sPAPRMachineState *spapr,
> sPAPRDIMMState *dimm_state)
> {
> - g_assert(!spapr_pending_dimm_unplugs_find(spapr, dimm_state->dimm));
> - QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
> + if (!spapr_pending_dimm_unplugs_find(spapr, dimm_state->dimm)) {
> + QTAILQ_INSERT_HEAD(&spapr->pending_dimm_unplugs, dimm_state, next);
> + }
> }
>
> static void spapr_pending_dimm_unplugs_remove(sPAPRMachineState *spapr,
--
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
signature.asc
Description: PGP signature
