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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 29 Feb 2016 19:33:15 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Igor Mammedov <imammedo@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 <armbru@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 <imammedo@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 <armbru@redhat.com>
> 
> Preferably with an improved commit message, of course :)
Thanks, I'll respin v2 with fixed commit message.