| Message ID | 1403021756-15960-12-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/17/2014 11:36 AM, Michael S. Tsirkin wrote: > From: Igor Mammedov <imammedo@redhat.com> > > using the same memdev backend more than once will cause > assersion at MemoryRegion mapping time becase it's already s/assersion/assertion/; s/becase/because/ (but probably too late now that this is a pull request) > mapped. Prevent it by checking that associated MemoryRegion > is not mapped. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Acked-by: Peter Crosthwaite <peter.crostwaite@xilinx.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > ---
On Tue, Jun 17, 2014 at 12:39:32PM -0600, Eric Blake wrote: > On 06/17/2014 11:36 AM, Michael S. Tsirkin wrote: > > From: Igor Mammedov <imammedo@redhat.com> > > > > using the same memdev backend more than once will cause > > assersion at MemoryRegion mapping time becase it's already > > s/assersion/assertion/; s/becase/because/ > (but probably too late now that this is a pull request) Absolutely too late. I don't mind spelling errors in commit logs too much, they don't show up in code. If noted in time I fix them in place ... > > mapped. Prevent it by checking that associated MemoryRegion > > is not mapped. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Acked-by: Peter Crosthwaite <peter.crostwaite@xilinx.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 17 June 2014 19:39, Eric Blake <eblake@redhat.com> wrote: > On 06/17/2014 11:36 AM, Michael S. Tsirkin wrote: >> From: Igor Mammedov <imammedo@redhat.com> >> >> using the same memdev backend more than once will cause >> assersion at MemoryRegion mapping time becase it's already > > s/assersion/assertion/; s/becase/because/ > (but probably too late now that this is a pull request) Incidentally, if anybody notices something that should mean a pull request ought not to be applied, they need to send an email which is a followup to the cover letter. Otherwise there's no guarantee I'll notice it when I'm processing the pullreq. thanks -- PMM
On 06/17/2014 12:45 PM, Peter Maydell wrote: > On 17 June 2014 19:39, Eric Blake <eblake@redhat.com> wrote: >> On 06/17/2014 11:36 AM, Michael S. Tsirkin wrote: >>> From: Igor Mammedov <imammedo@redhat.com> >>> >>> using the same memdev backend more than once will cause >>> assersion at MemoryRegion mapping time becase it's already >> >> s/assersion/assertion/; s/becase/because/ >> (but probably too late now that this is a pull request) > > Incidentally, if anybody notices something that should > mean a pull request ought not to be applied, they need > to send an email which is a followup to the cover letter. > Otherwise there's no guarantee I'll notice it when I'm > processing the pullreq. Got it. So far, all that I've pointed out can be done as followups (typos in code) or ignored as too late (typos in commit message) - I haven't found anything that should prevent this pull request from actually going in.
On Tue, Jun 17, 2014 at 07:45:10PM +0100, Peter Maydell wrote: > On 17 June 2014 19:39, Eric Blake <eblake@redhat.com> wrote: > > On 06/17/2014 11:36 AM, Michael S. Tsirkin wrote: > >> From: Igor Mammedov <imammedo@redhat.com> > >> > >> using the same memdev backend more than once will cause > >> assersion at MemoryRegion mapping time becase it's already > > > > s/assersion/assertion/; s/becase/because/ > > (but probably too late now that this is a pull request) > > Incidentally, if anybody notices something that should > mean a pull request ought not to be applied, they need > to send an email which is a followup to the cover letter. > Otherwise there's no guarantee I'll notice it when I'm > processing the pullreq. > > thanks > -- PMM Right. Though that's kind of creating the wrong incentive for people - why bother reviewing patches on list, I can check what's being pulled ;) So note it needs to be something very serious, like a crash for most users on a major platform. Issues that affect minority of users, warnings, error handling errors can be fixed in follow up patches. And sending Acks in response to the pull request is definitely not helpful :)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index b4937fe..3cced63 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -43,6 +43,21 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, void *opaque, visit_type_int(v, &value, name, errp); } +static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name, + Object *val, Error **errp) +{ + MemoryRegion *mr; + + mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), errp); + 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); + g_free(path); + } else { + qdev_prop_allow_set_link_before_realize(obj, name, val, errp); + } +} + static void pc_dimm_init(Object *obj) { PCDIMMDevice *dimm = PC_DIMM(obj); @@ -51,7 +66,7 @@ static void pc_dimm_init(Object *obj) NULL, NULL, NULL, &error_abort); object_property_add_link(obj, PC_DIMM_MEMDEV_PROP, TYPE_MEMORY_BACKEND, (Object **)&dimm->hostmem, - qdev_prop_allow_set_link_before_realize, + pc_dimm_check_memdev_is_busy, OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort); }