Re: [systemd-devel] [RFC] bus: add sd_bus_emit_object_{added, removed}()
Hi On Tue, Feb 18, 2014 at 12:02 AM, David Herrmann dh.herrm...@gmail.com wrote: The ObjectManager dbus interface provides an InterfacesAdded signal to notify others about new interfaces that are added to an object. The same signal is also used to advertise new objects (by adding the first interface to a given object path) and delete them. However, our internal helpers sd_bus_emit_interfaces_{added,removed}() cannot properly deal with built-in interfaces like DBus.Properties as there's no vtable for it. Therefore, to avoid callers to track these internal interfaces, we provide two separate helpers which explicitly add these interfaces to the signal. sd_bus_emit_object_added() traverses the list of all vtables and fallback vtables (making sure a fallback never overwrites a real vtable!) and also adds built-in interfaces. sd_bus_emit_object_removed(): WIP --- Hi This is untested and I just wanted to get some feedback whether that's the way to go. Given the previous discussion we decided on two new entry-points to add and remove objects. For convenience, I now tried to omit any char **interfaces argument to object_added() and just try to figure them out on my own. However, on object_removed() I cannot do that as node_vtable_get_userdata() is very likely to return 0 for all these objects. So there is no way to figure out which interfaces actually existed on that thing. We would require users to call it *before* destroying/unlinking the actual object. I don't know whether that's ok to assume? If not, we can just add a char **interfaces argument, but then it would differ from object_added().. Not sure what sounds better.. Cheers David src/libsystemd/sd-bus/bus-objects.c | 210 src/systemd/sd-bus.h| 2 + 2 files changed, 212 insertions(+) After almost 1 year of niche existence I finally went ahead and re-implemented this. Review is welcome! Pushed. Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] bus: add sd_bus_emit_object_{added, removed}()
On Tue, 18.02.14 00:02, David Herrmann (dh.herrm...@gmail.com) wrote: Sorry for the late review! However, on object_removed() I cannot do that as node_vtable_get_userdata() is very likely to return 0 for all these objects. So there is no way to figure out which interfaces actually existed on that thing. We would require users to call it *before* destroying/unlinking the actual object. I don't know whether that's ok to assume? I would assume that it is OK to assume that. People should call this function before half-destroying their object. I mean we are not reading the properties after all, just the interfaces and I think that should be quite OK to require. Certainly something to document though one day... +static int object_added_append_all_prefix( +sd_bus *bus, +sd_bus_message *m, +Set *s, +const char *prefix, +const char *path, +bool require_fallback) { + +if (!streq_ptr(c-interface, previous_interface)) { +/* interface already handled by a previous run? */ +if (set_get(s, c-interface)) +continue; We actually allow multiple vtables with the same interface, and order them together in the vtable list, so that we can iterate through them easily with trivial duplicate reduction. Keeping a Set object here appears unnecessary? Or did I miss something here? Otherwise looks good! Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] bus: add sd_bus_emit_object_{added, removed}()
Hi On Tue, Mar 11, 2014 at 7:27 PM, Lennart Poettering lenn...@poettering.net wrote: On Tue, 18.02.14 00:02, David Herrmann (dh.herrm...@gmail.com) wrote: Sorry for the late review! However, on object_removed() I cannot do that as node_vtable_get_userdata() is very likely to return 0 for all these objects. So there is no way to figure out which interfaces actually existed on that thing. We would require users to call it *before* destroying/unlinking the actual object. I don't know whether that's ok to assume? I would assume that it is OK to assume that. People should call this function before half-destroying their object. I mean we are not reading the properties after all, just the interfaces and I think that should be quite OK to require. Certainly something to document though one day... Yepp, will add docs once I commit it. +static int object_added_append_all_prefix( +sd_bus *bus, +sd_bus_message *m, +Set *s, +const char *prefix, +const char *path, +bool require_fallback) { + +if (!streq_ptr(c-interface, previous_interface)) { +/* interface already handled by a previous run? */ +if (set_get(s, c-interface)) +continue; We actually allow multiple vtables with the same interface, and order them together in the vtable list, so that we can iterate through them easily with trivial duplicate reduction. Keeping a Set object here appears unnecessary? Or did I miss something here? A single call to object_added_append_all_prefix() doesn't need Set *s as it's ordered (as you said). The problem is more subtle, see object_added_append_all(), where I do this: +r = object_added_append_all_prefix(bus, m, s, path, path, false); +if (r 0) +return r; +if (bus-nodes_modified) +return 0; + +prefix = alloca(strlen(path) + 1); +OBJECT_PATH_FOREACH_PREFIX(prefix, path) { +r = object_added_append_all_prefix(bus, m, s, prefix, path, true); +if (r 0) +return r; +if (bus-nodes_modified) +return 0; +} First I call append_all_prefix() with the exact path, which cannot ever fire the if (set_get()) protection, as it's the first run. However, the prefix-runs which I do afterwards will traverse the parents, which might *also* implement the interface. They use require_fallback == true, so usually we're fine. However, if you implement an interface explicitly on a child, but as fallback on the parents, I _must_ avoid adding the interface twice. At least that's my understanding of the vtable API, or should these be merged? I looked at interfaces_added_append_one(), which skips parents if the node already implements the interface. Therefore, I assumed I have to skip parents, too. But I still need to traverse the parents, because they might implement different interfaces that I haven't found, yet. Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC] bus: add sd_bus_emit_object_{added, removed}()
ping? On Tue, Feb 18, 2014 at 12:02 AM, David Herrmann dh.herrm...@gmail.com wrote: The ObjectManager dbus interface provides an InterfacesAdded signal to notify others about new interfaces that are added to an object. The same signal is also used to advertise new objects (by adding the first interface to a given object path) and delete them. However, our internal helpers sd_bus_emit_interfaces_{added,removed}() cannot properly deal with built-in interfaces like DBus.Properties as there's no vtable for it. Therefore, to avoid callers to track these internal interfaces, we provide two separate helpers which explicitly add these interfaces to the signal. sd_bus_emit_object_added() traverses the list of all vtables and fallback vtables (making sure a fallback never overwrites a real vtable!) and also adds built-in interfaces. sd_bus_emit_object_removed(): WIP --- Hi This is untested and I just wanted to get some feedback whether that's the way to go. Given the previous discussion we decided on two new entry-points to add and remove objects. For convenience, I now tried to omit any char **interfaces argument to object_added() and just try to figure them out on my own. However, on object_removed() I cannot do that as node_vtable_get_userdata() is very likely to return 0 for all these objects. So there is no way to figure out which interfaces actually existed on that thing. We would require users to call it *before* destroying/unlinking the actual object. I don't know whether that's ok to assume? If not, we can just add a char **interfaces argument, but then it would differ from object_added().. Not sure what sounds better.. Cheers David src/libsystemd/sd-bus/bus-objects.c | 210 src/systemd/sd-bus.h| 2 + 2 files changed, 212 insertions(+) diff --git a/src/libsystemd/sd-bus/bus-objects.c b/src/libsystemd/sd-bus/bus-objects.c index b116a5d..0c099a3 100644 --- a/src/libsystemd/sd-bus/bus-objects.c +++ b/src/libsystemd/sd-bus/bus-objects.c @@ -2469,6 +2469,216 @@ _public_ int sd_bus_emit_interfaces_removed(sd_bus *bus, const char *path, const return sd_bus_emit_interfaces_removed_strv(bus, path, interfaces); } +static int object_added_append_all_prefix( +sd_bus *bus, +sd_bus_message *m, +Set *s, +const char *prefix, +const char *path, +bool require_fallback) { + +_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL; +const char *previous_interface = NULL; +struct node_vtable *c; +struct node *n; +void *u = NULL; +int r; + +assert(bus); +assert(m); +assert(s); +assert(prefix); +assert(path); + +n = hashmap_get(bus-nodes, prefix); +if (!n) +return 0; + +LIST_FOREACH(vtables, c, n-vtables) { +if (require_fallback !c-is_fallback) +continue; + +r = node_vtable_get_userdata(bus, path, c, u, error); +if (r 0) +return r; +if (bus-nodes_modified) +return 0; +if (r == 0) +continue; + +if (!streq_ptr(c-interface, previous_interface)) { +/* interface already handled by a previous run? */ +if (set_get(s, c-interface)) +continue; + +/* prevent fallbacks from overwriting specific objs */ +r = set_put(s, c-interface); +if (r 0) +return r; + +if (previous_interface) { +r = sd_bus_message_close_container(m); +if (r 0) +return r; + +r = sd_bus_message_close_container(m); +if (r 0) +return r; +} + +r = sd_bus_message_open_container(m, 'e', sa{sv}); +if (r 0) +return r; + +r = sd_bus_message_append_basic(m, 's', c-interface); +if (r 0) +return r; + +r = sd_bus_message_open_container(m, 'a', {sv}); +if (r 0) +return r; + +previous_interface = c-interface; +} + +r = vtable_append_all_properties(bus, m, path, c, u, error); +if (r 0) +return r; +