On Mon, Apr 27, 2015 at 11:46 PM, Eric Auger <eric.auger@xxxxxxxxxx> wrote: > Hi Peter, > > On 04/27/2015 07:43 PM, Peter Crosthwaite wrote: >> On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: >>> >>> >>> On 27/04/2015 16:56, Eric Auger wrote: >>>> Peter, Paolo, >>>> >>>> After your feedbacks, I feel I need to spend some more time on the >>>> original check() track. I would prefer not to introduce any patch that >>>> will make issue in the future. >>> >>> Peter, see the other threads between me and Eric. See in particular >>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html >>> starting at "The notifier actually is not even necessary" and the >>> replies from there. >>> >> >> Thanks, >> >> I see the problem with check. In this reply >> >> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html >> >> Eric says that the problem with the check hook is it happens before >> the setting. I think this can be solved with a RYO link setter for >> GPIOs. We almost have an in-tree precedent with MemoryRegion and the >> container property (memory.c): >> >> 960 op = object_property_add(OBJECT(mr), "container", >> 961 "link<" TYPE_MEMORY_REGION ">", >> 962 memory_region_get_container, >> 963 NULL, /* memory_region_set_container */ >> 964 NULL, NULL, &error_abort); >> >> Now in reality we could have done this link normal style as it is only >> a trivial getter, but the reason the link was done this way in the >> first place, is because I have a follow up patch to memory.c that adds >> a customer Link setter: >> >> +static void memory_region_set_container(Object *obj, Visitor *v, void *opaque, >> + const char *name, Error **errp) >> +{ >> + MemoryRegion *mr = MEMORY_REGION(obj); >> + Error *local_err = NULL; >> + MemoryRegion *old_container = mr->container; >> + MemoryRegion *new_container = NULL; >> + char *path = NULL; >> + >> + visit_type_str(v, &path, name, &local_err); >> + >> + if (!local_err && strcmp(path, "") != 0) { >> + new_container = MEMORY_REGION(object_resolve_link(obj, name, path, >> + &local_err)); >> + while (new_container->alias) { >> + new_container = new_container->alias; >> + } >> + } >> + >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + >> + object_ref(OBJECT(new_container)); >> + >> + memory_region_transaction_begin(); >> + memory_region_ref(mr); >> + if (old_container) { >> + memory_region_del_subregion(old_container, mr); >> + } >> + mr->container = new_container; >> + if (new_container) { >> + memory_region_update_container_subregions(mr); >> + } >> + memory_region_unref(mr); >> + memory_region_transaction_commit(); >> + >> + object_unref(OBJECT(old_container)); >> +} >> + >> >> op = object_property_add(OBJECT(mr), "container", >> "link<" TYPE_MEMORY_REGION ">", >> memory_region_get_container, >> - NULL, /* memory_region_set_container */ >> + memory_region_set_container, >> NULL, NULL, &error_abort); >> >> >> The function does the normal link setting - similar to >> object_set_link_property (not to be confused with >> object_property_set_link!) but is surrounded by class specific side >> effects. Specifically in this case, it does >> memory_region_transaction_begin/ref/unref/commit etc for the MR. >> >> For this GPIO case, you could create a custom setter that does the >> normal set, then calls the DeviceClass installed hook (or you can >> install the hook to the object and init it at qdev_init_gpio_out_named >> time as suggested in the eariler thread). The callback will happen >> after the link is populated. >> >> To reduce verbosity, I suggest making object_set_link_property() a >> visible API, then RYO link setters can call it surrounded by custom >> behavior e.g: >> >> foo_object_set_bar_property(...) >> { >> pre_set_link_side_effects(); >> object_set_link_property(); >> post_set_link_side_effects(); >> } >> >> object_set_link_property() would need to be coreified and wrapped to >> remove it's awareness of LinkProperty type (as that doesn't exist in >> RYO properties) in this case. > > Thank you Peter for detailing this. > > Yesterday I re-worked on the solution based on the check() method where > - check would take a Object **child as a 3d parameter > - we would assign *child before the call and in case the check fails set > the *child back to NULL in object_set_link_property. > I think this is starting to reach outside the design intent of the check, letting it be an API that takes over the responsibility of ->set. Ideally check should be boolean and side-effectless. > I need to do some more testing. > > I don't know if this solution would be acceptable too. If not I will > implement according to your guidelines. > Lets see what Paolo says before another rework. Regards, Peter > Best Regards > > Eric >> >> Regards, >> Peter >> >>> If you have any idea, please help. >>> >>> Paolo >>> > > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
