Igor Mammedov <imamm...@redhat.com> 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 <arm...@redhat.com>
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 <imamm...@redhat.com>
> ---
> 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 <arm...@redhat.com>
Preferably with an improved commit message, of course :)
