Re: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev'

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

On Thu, Aug 17, 2017 at 08:33:10PM +0200, Thomas Huth wrote:
> QEMU currently crashes when trying to use a 'pc-dimm' on the pseries
> machine without specifying its 'memdev' property. Let's add a sanity
> check to the pre_plug handler to fix this issue.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks for all these patches fixing little bugs in 2.10.

> ---
>  hw/ppc/spapr.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f7a1972..22d400a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2808,10 +2808,17 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> -    MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t size = memory_region_size(mr);
> +    MemoryRegion *mr;
> +    uint64_t size;
>      char *mem_dev;
>  
> +    if (!dimm->hostmem) {

Isn't checking dimm->hostmem directly here an abstraction violation?
Could we just check for a NULL return from get_memory_region instead?


> +        error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
> +        return;
> +    }
> +
> +    mr = ddc->get_memory_region(dimm);
> +    size = memory_region_size(mr);
>      if (size % SPAPR_MEMORY_BLOCK_SIZE) {
>          error_setg(errp, "Hotplugged memory size must be a multiple of "
>                        "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]