Re: [Qemu-devel] [PATCH] pc-dimm: fix error handling in pc_dimm_check_memdev_is_busy()

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> if host_memory_backend_get_memory() were to return error and

Start sentences with a capital letter, please.

> NULL MemoryRegion, pc_dimm_check_memdev_is_busy() would crash
> dereferrencing null pointer in memory_region_is_mapped()

dereferencing

>
> Also pc_dimm_check_memdev_is_busy():error_setg() would assert
> if caller passes NULL errp, but assert shouldn't happen as
> the check is typically performed during hotplug.

Huh?

>
> To avoid above issues use typical error handling pattern
> for property setters:
>
> Error *local_error = NULL;
> ...
> error_propagate(errp, local_err);
>
> Reported-by: Markus Armbruster <address@hidden>

The latent bug I reported was actually that if
host_memory_backend_get_memory() sets an error and we then reach
error_setg(), we fail the "error already set" assertion in error_setv()
unless errp is null.

> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/mem/pc-dimm.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 650f0f8..973bf20 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -364,15 +364,22 @@ static void pc_dimm_check_memdev_is_busy(Object *obj, 
> const char *name,
>                                        Object *val, Error **errp)
>  {
>      MemoryRegion *mr;
> +    Error *local_err = NULL;
>  
> -    mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), errp);
> +    mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
>      if (memory_region_is_mapped(mr)) {
>          char *path = object_get_canonical_path_component(val);
> -        error_setg(errp, "can't use already busy memdev: %s", path);
> +        error_setg(&local_err, "can't use already busy memdev: %s", path);
>          g_free(path);
>      } else {
> -        qdev_prop_allow_set_link_before_realize(obj, name, val, errp);
> +        qdev_prop_allow_set_link_before_realize(obj, name, val, &local_err);
>      }
> +
> +out:
> +    error_propagate(errp, local_err);
>  }
>  
>  static void pc_dimm_init(Object *obj)

I'd error_propagate() + return instead of goto.  But your version isn't
wrong, so:

Reviewed-by: Markus Armbruster <address@hidden>

Preferably with an improved commit message, of course :)