On Mon, 29 Feb 2016 19:33:15 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> 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?
Yep, this paragraph is wrong, I'll drop it.
>
> >
> > 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 :)
Thanks, I'll respin v2 with fixed commit message.
