GNOME Bugzilla – Bug 771191
[review] lr/object-manager: libnm should use the ObjectManager API
Last modified: 2016-11-14 12:40:39 UTC
Work in progress: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/om
Ready for review: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/object-manager
+ for (i = 0; i < odata->length; i++) { add_to_object_array_unique (new, odata->objects[i]); + } braces. + GList *this; usually we call this variable @iter. @this makes me thing of C++. odata->objects = g_new (GObject *, 1); I don't see where objects[0] gets set. Can you change it to g_new0()? /* Only complete the array property load when all the objects are initialized. */ for (i = 0; i < odata->length; i++) { GObject *obj = odata->objects[i]; NMObjectPrivate *obj_priv; /* Could not load the object. Perhaps it was removed. */ if (!obj) continue; Is it not possible that various @obj are NULL, because initialization didn't start yet? I think, you you have to put a check before the loop: if (odata->remaining > 0) return; + /* The odata may hold the last reference. */ + g_object_ref (self); leaks due to "return" in /* Only complete the array property load when all the objects are initialized. */. Just do _nm_unused gs_unref_object NMObject *self_keep_alive = g_object_ref (self);
assertion failure in `make check`: nm_assert (!g_strcmp0 (g_dbus_proxy_get_name (proxy), NM_DBUS_SERVICE)); (compile with --with-more-asserts)
The code doc for the NM_OBJECT_DBUS_OBJECT_MANAGER property in nm-object.c is a C&P of the code doc for NM_OBJECT_DBUS_OBJECT. I think this should be 'const' too: -static GPtrArray empty = { 0, }; +static const GPtrArray empty = { 0, }; Lastly, it seems libnm grows significantly with the last commit "libnm: use the o.fd.DBus.ObjectManager API for object management". Before (stripped): 884072 After (stripped): 1023624 -------------------------- 139552 That's a pretty big number, do we have any idea where it's coming from? I feel like a ton of code got removed, and not as much code got added, so this is unexpected. Sections (via readelf -SaW) that increased greatly include: text 446581 -> 505846 (executable code) eh_frame 117404 -> 147356 data.rel.ro 36880 -> 52104 rela.dyn 51792 -> 71064 It looks like somehow our code size jumped a ton, not sure why, maybe we're now inlining something in a lot more places than before?
(In reply to Thomas Haller from comment #2) > + for (i = 0; i < odata->length; i++) { > add_to_object_array_unique (new, odata->objects[i]); > + } Fixed > + GList *this; > > usually we call this variable @iter. @this makes me thing of C++. That's horrible. Nobody should be made think for C++. Fixed. > odata->objects = g_new (GObject *, 1); > > I don't see where objects[0] gets set. Can you change it to g_new0()? Fixed. > /* Only complete the array property load when all the objects are > initialized. */ > for (i = 0; i < odata->length; i++) { > GObject *obj = odata->objects[i]; > NMObjectPrivate *obj_priv; > > /* Could not load the object. Perhaps it was removed. */ > if (!obj) > continue; > > > Is it not possible that various @obj are NULL, because initialization didn't > start yet? I think, you you have to put a check before the loop: > > if (odata->remaining > 0) > return; Right. Fixed. > + /* The odata may hold the last reference. */ > + g_object_ref (self); > > leaks due to "return" in /* Only complete the array property load when all > the objects are initialized. */. > > Just do > > _nm_unused gs_unref_object NMObject *self_keep_alive = g_object_ref > (self); Okay. (In reply to Thomas Haller from comment #3) > assertion failure in `make check`: > > nm_assert (!g_strcmp0 (g_dbus_proxy_get_name (proxy), NM_DBUS_SERVICE)); > > (compile with --with-more-asserts) Hm, this is because now the proxy is constructed by the GDBusObjectManagerClient and it uses the unique name ":<number>" as opposed to a well-known bus name. I guess it's okay to just drop the assert. (In reply to Dan Williams from comment #4) > The code doc for the NM_OBJECT_DBUS_OBJECT_MANAGER property in nm-object.c > is a C&P of the code doc for NM_OBJECT_DBUS_OBJECT. > > I think this should be 'const' too: > > -static GPtrArray empty = { 0, }; > +static const GPtrArray empty = { 0, }; Fixed. > Lastly, it seems libnm grows significantly with the last commit "libnm: use > the o.fd.DBus.ObjectManager API for object management". > > Before (stripped): 884072 > After (stripped): 1023624 > -------------------------- > 139552 > > That's a pretty big number, do we have any idea where it's coming from? I > feel like a ton of code got removed, and not as much code got added, so this > is unexpected. > > Sections (via readelf -SaW) that increased greatly include: > > text 446581 -> 505846 (executable code) > eh_frame 117404 -> 147356 > data.rel.ro 36880 -> 52104 > rela.dyn 51792 -> 71064 > > It looks like somehow our code size jumped a ton, not sure why, maybe we're > now inlining something in a lot more places than before? This: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=lr/object-manager&id=c3a8104ff2bda36c4301d3c3648cbb291ec44eee I've left the secret agent conversion to the generated bindings in place, because the code looks better. Moved it to a separate commit though. (Pushed an updated branch for review.)
LGTM, but could you add a comment in proxy_type() to explain why only some proxies are returned now and not others, just so we don't forget? Might also say that each proxy added to this function greatly increases code size due to inlining (or whatever) and so we want to keep it small.
interfaces = g_dbus_object_get_interfaces (priv->object); ^^^ indention You are using a GList, which also has a "prev" pointer. I don't think that is used, maybe just use GSList instead. I guess, NMObject:dipose() is never reached with a non-empty priv->pending list. If that is the case, could we add there a: nm_assert (!priv->pending); typos in commit message: property changes so tha twe don't have to and lets us always see a ^^^^^^^ etc.) is had to be moded to NMClient. The bright side is that his allows ^^^ ^^^^^ ^^^ typo in code-comment: in between it seen sees the service disappear and the call to ^^^^^^^^^ - g_signal_connect (priv->proxy, "state-changed", - G_CALLBACK (device_state_changed), object); + g_signal_connect (priv->proxy, "notify::state-reason", + G_CALLBACK (device_state_reason_changed), object); why do you change it to react on notify::state-reason? It seems, sometimes the state-reason keeps to be the same "CONNECTION_ASSUMED", while the state goes through the activation states. This at least warrants some comment. _("Active connection removed before it was initialized xx")); ^^^^^^^^^ libnm/nm-manager.c: g_object_get (self, NM_OBJECT_DBUS_OBJECT_MANAGER, &object_manager, NULL); for one, this leaks object_manager. But maybe just add an internal accessor to avoid the overhead of looking up the property by name, passing it through a GValue, ref/unref the value. object_manager = nm_object_get_dbus_object_manager (NM_OBJECT (self)); + obj = g_object_get_data (G_OBJECT (object), "nm-object"); object_created (obj, path, odata); + g_object_unref (object); It's not clear why you unref the object here. g_object_get_data() does not increase the re-count. If you want to take the object out, use g_object_steal_data(). Otherwise, this looks like a dangling pointer. Could we use instead g_object_get_qdata() and a quark? + priv->proxy = NMDBUS_SETTINGS_CONNECTION (_nm_object_get_proxy (NM_OBJECT (initable), NM_DBUS_INTERFACE_SETTINGS_CONNECTION)); + g_assert (priv->proxy); twice you do: + priv->proxy = [...] (_nm_object_get_proxy (NM_OBJ... + g_assert (priv->proxy); note that _nm_object_get_proxy already has a (graceful) assertion g_return_val_if_fail (proxy != NULL, NULL); so it seems this is overkill. (and it's not clear to me that a SIGABORT is preferred then a g_critical() in the unexpected case). Drop g_assert() or use nm_assert() instead. + if (!g_dbus_object_manager_client_get_name_owner (G_DBUS_OBJECT_MANAGER_CLIENT (object))) { leaks name-owner. + g_dbus_object_manager_client_new_for_bus (_nm_dbus_bus_type (), + G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, + "org.freedesktop.NetworkManager", + "/org/freedesktop", + proxy_type, NULL, NULL, + NULL, + got_object_manager, + init_data); I think this should use "init_data->cancellable". as it is, prepare_object_manager() does init_data->result = g_simple_async_result_new (G_OBJECT (client), which takes a ref on @client. Thus, client will stay alive long enough until got_object_manager() completes. Ok, that is correct. But in general, I think it's not great if an async operation keeps the source-object alive. I think if the user destroys the object while the async operation is in progress, it should cancel the operation and not crash. IOW, that means - got_object_manager() must not touch @self/@client until it calls g_dbus_object_manager_client_new_for_bus_finish() and checks g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED) - a caller of prepare_object_manager() must either pass a cancellable *or* take a reference on self (or both). +static void +name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) +{ + ... + GDBusObjectManager *object_manager = G_DBUS_OBJECT_MANAGER (object); ... + g_object_unref (object_manager); seems wrong. have unhook_om() freeze/thaw_notify()? + g_printerr ("><OBJECT INITED BADLY THO FFF>> %s <<<\n", error->message);
(In reply to Dan Williams from comment #6) > LGTM, but could you add a comment in proxy_type() to explain why only some > proxies are returned now and not others, just so we don't forget? Might > also say that each proxy added to this function greatly increases code size > due to inlining (or whatever) and so we want to keep it small. Added a comment. (In reply to Thomas Haller from comment #7) > interfaces = g_dbus_object_get_interfaces (priv->object); > > ^^^ indention Fixed. > You are using a GList, which also has a "prev" pointer. I don't think that > is used, maybe just use GSList instead. Well, that's GLib API... > I guess, NMObject:dipose() is never reached with a non-empty priv->pending > list. If that is the case, could we add there a: > > nm_assert (!priv->pending); Why? > typos in commit message: > > property changes so tha twe don't have to and lets us always see a > ^^^^^^^ > etc.) is had to be moded to NMClient. The bright side is that his allows > ^^^ ^^^^^ ^^^ Fixed. > typo in code-comment: > > in between it seen sees the service disappear and the call to > ^^^^^^^^^ Fixed > - g_signal_connect (priv->proxy, "state-changed", > - G_CALLBACK (device_state_changed), object); > + g_signal_connect (priv->proxy, "notify::state-reason", > + G_CALLBACK (device_state_reason_changed), object); > > why do you change it to react on notify::state-reason? It seems, sometimes > the state-reason keeps to be the same "CONNECTION_ASSUMED", while the state > goes through the activation states. This at least warrants some comment. Because with object-manager, the state-changed is emmitted before the GDBusObject would have cached the new properties, thus the object doesn't really reflect the situation after the state change. > _("Active connection removed before it was initialized xx")); Fixed. > libnm/nm-manager.c: > g_object_get (self, NM_OBJECT_DBUS_OBJECT_MANAGER, &object_manager, NULL); > for one, this leaks object_manager. > > But maybe just add an internal accessor to avoid the overhead of looking up > the property by name, passing it through a GValue, ref/unref the value. > object_manager = nm_object_get_dbus_object_manager (NM_OBJECT (self)); Added the internal accessor. > + obj = g_object_get_data (G_OBJECT (object), "nm-object"); > object_created (obj, path, odata); > + g_object_unref (object); > > It's not clear why you unref the object here. g_object_get_data() does not > increase the re-count. If you want to take the object out, use > g_object_steal_data(). Otherwise, this looks like a dangling pointer. Uh, not sure either. It shouldn't be there. Removed. Wondering why didn't it cause a crash. > Could we use instead g_object_get_qdata() and a quark? Done. > + priv->proxy = NMDBUS_SETTINGS_CONNECTION (_nm_object_get_proxy > (NM_OBJECT (initable), NM_DBUS_INTERFACE_SETTINGS_CONNECTION)); > + g_assert (priv->proxy); > > > > twice you do: > + priv->proxy = [...] (_nm_object_get_proxy (NM_OBJ... > + g_assert (priv->proxy); > > note that _nm_object_get_proxy already has a (graceful) assertion > g_return_val_if_fail (proxy != NULL, NULL); > so it seems this is overkill. (and it's not clear to me that a SIGABORT is > preferred then a g_critical() in the unexpected case). Drop g_assert() or > use nm_assert() instead. Dropped. > + if (!g_dbus_object_manager_client_get_name_owner > (G_DBUS_OBJECT_MANAGER_CLIENT (object))) { > > leaks name-owner. Fixed. > + g_dbus_object_manager_client_new_for_bus (_nm_dbus_bus_type (), > + > G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, > + > "org.freedesktop.NetworkManager", > + "/org/freedesktop", > + proxy_type, NULL, NULL, > + NULL, > + got_object_manager, > + init_data); > > I think this should use "init_data->cancellable". Fixed. > as it is, prepare_object_manager() does > init_data->result = g_simple_async_result_new (G_OBJECT (client), > which takes a ref on @client. Thus, client will stay alive long enough until > got_object_manager() completes. Ok, that is correct. > > But in general, I think it's not great if an async operation keeps the > source-object alive. I think if the user destroys the object while the async > operation is in progress, it should cancel the operation and not crash. > IOW, that means > - got_object_manager() must not touch @self/@client until it calls > g_dbus_object_manager_client_new_for_bus_finish() and checks g_error_matches > (error, G_IO_ERROR, G_IO_ERROR_CANCELLED) > - a caller of prepare_object_manager() must either pass a cancellable *or* > take a reference on self (or both). Still added cancellation stack for name_owner_changed()->new_object_manager() so that we cancel the object managers on quick subsequent daemon restarts. > +static void > +name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) > +{ > + ... > + GDBusObjectManager *object_manager = G_DBUS_OBJECT_MANAGER (object); > ... > + g_object_unref (object_manager); > > seems wrong. Restructured to look better. > have unhook_om() freeze/thaw_notify()? Why? > + g_printerr ("><OBJECT INITED BADLY THO FFF>> %s <<<\n", > error->message); Whoops, a leftover. Removed. Updated the branch.
Added a fixup for GList -> GSList thing.
static const GPtrArray empty = { 0, }; A user might be compelled to take a reference on the returned object. That will not work with @empty (ok, taking an additional ref on a "const GPtrArray *" can be considered a bug and is not allowed). Still... blame it on GPtrArray which doesn't provision a stack-allocated version, but doing such hacks seems like calling for trouble. actually use quote some memory ^^^^^ /* An object proxy: explain. */ ^^^^^^^ IMO static function shouldn't have an nm-prefix (nm_object_for_gdbus_object). Same for local variables (NMObject *nm_object;). On the other hand, stuff in header files should have nm-prefixes. It's convenient to look at code and immediately see whether it is something from an nm-header file. (yes, nmtui doesn't follow that, so apparently there is disagreement about that convention). Sure, we can come up with different conventions if you dislike that convention. Then could you comment what you would prefer instead? For example, a better name for "nm_object_inited" would be object_added_cb. I mean, naming in just so important, otherwise it's hopeless to understand those amounts of LOC. + g_list_foreach (objects, (GFunc) remove_nm_object_for_gdbus_object, (gpointer) priv->object_manager); + g_list_free_full (objects, g_object_unref); the priv->object_manager argument is unused by remove_nm_object_for_gdbus_object(). You could combine these two functions in one g_list_free_full (object, _destroy_obj); or maybe just open-code it, it's less LOC and all the behavior is in one place. libnm/nm-object.h is public API. +GQuark nm_object_quark (void); + +GType nm_object_get_interface_proxy_type (const char *interface); nm_object_quark shall not be public API. nm_object_get_interface_proxy_type() seems to not exist? +#include <nmdbus-device-olpc-mesh.h> systemd uses everywhere "" for including internal headers. Even in their public header files, they use "". E.g. sd-login does #include "_sd-common.h" I think that is actually more correct. Let's use "" everywhere (at least for new code). + g_signal_connect (object_manager, "object-added", + G_CALLBACK (object_added), client); +object_added (GDBusObjectManager *object_manager, GDBusObject *object, gpointer user_data) + g_async_initable_init_async (G_ASYNC_INITABLE (nm_object), + G_PRIORITY_DEFAULT, NULL, + nm_object_inited, user_data); nm_object_initiated doesn't user user_data/client. Don't pass it on. Same for + g_signal_connect (object_manager, "object-removed", + G_CALLBACK (object_removed), client); or are you doing that because of g_signal_handlers_disconnect_by_data (priv->object_manager, object); ? Non-obvious, please add a code comment. + if (priv->new_object_manager_cancellable) { + g_cancellable_cancel (priv->new_object_manager_cancellable); + g_clear_object (&priv->new_object_manager_cancellable); + } nm_clear_g_cancellable (&priv->new_object_manager_cancellable); valgrind seems to find some memleaks.
(In reply to Thomas Haller from comment #10) > static const GPtrArray empty = { 0, }; > > A user might be compelled to take a reference on the returned object. That > will not work with @empty (ok, taking an additional ref on a "const > GPtrArray *" can be considered a bug and is not allowed). Still... blame it > on GPtrArray which doesn't provision a stack-allocated version, but doing > such hacks seems like calling for trouble. Yeah, but you shouldn't be referencing an object you don't own. I think it would still be an unwanted regression if we made anything important crash because of this, but I don't think it is likely. Let's change it even if we learn of anything like that. > actually use quote some memory > ^^^^^ Fixed. > /* An object proxy: explain. */ > ^^^^^^^ Fixed. > IMO static function shouldn't have an nm-prefix > (nm_object_for_gdbus_object). Same for local variables (NMObject > *nm_object;). > On the other hand, stuff in header files should have nm-prefixes. It's > convenient to look at code and immediately see whether it is something from > an nm-header file. > (yes, nmtui doesn't follow that, so apparently there is disagreement about > that convention). > Sure, we can come up with different conventions if you dislike that > convention. Then could you comment what you would prefer instead? > > For example, a better name for "nm_object_inited" would be object_added_cb. > I mean, naming in just so important, otherwise it's hopeless to understand > those amounts of LOC. Fixed. > + g_list_foreach (objects, (GFunc) remove_nm_object_for_gdbus_object, > (gpointer) priv->object_manager); > + g_list_free_full (objects, g_object_unref); > > the priv->object_manager argument is unused by > remove_nm_object_for_gdbus_object(). You could combine these two functions > in one > g_list_free_full (object, _destroy_obj); > or maybe just open-code it, it's less LOC and all the behavior is in one > place. I can't see why would that be better. > libnm/nm-object.h is public API. > > +GQuark nm_object_quark (void); > + > +GType nm_object_get_interface_proxy_type (const char *interface); > > nm_object_quark shall not be public API. Fixed. > nm_object_get_interface_proxy_type() seems to not exist? Fixed. > +#include <nmdbus-device-olpc-mesh.h> > > systemd uses everywhere "" for including internal headers. Even in their > public header files, they use "". E.g. sd-login does #include "_sd-common.h" > I think that is actually more correct. Let's use "" everywhere (at least for > new code). Fixed. > + g_signal_connect (object_manager, "object-added", > + G_CALLBACK (object_added), client); > > +object_added (GDBusObjectManager *object_manager, GDBusObject *object, > gpointer user_data) > + g_async_initable_init_async (G_ASYNC_INITABLE (nm_object), > + G_PRIORITY_DEFAULT, NULL, > + nm_object_inited, user_data); > > nm_object_initiated doesn't user user_data/client. Don't pass it on. > > Same for > > + g_signal_connect (object_manager, "object-removed", > + G_CALLBACK (object_removed), client); > > > > or are you doing that because of > > g_signal_handlers_disconnect_by_data (priv->object_manager, object); > > ? Non-obvious, please add a code comment. Added a comment. Removed the useless use in object_added(). > + if (priv->new_object_manager_cancellable) { > + g_cancellable_cancel (priv->new_object_manager_cancellable); > + g_clear_object (&priv->new_object_manager_cancellable); > + } > > nm_clear_g_cancellable (&priv->new_object_manager_cancellable); > > > > valgrind seems to find some memleaks. Can't see any. Let's re-try when "make check" with valgrind is fixed. Pushed some fixups.
Merged in 93fa62c2bf554d418bb809682cde8b4709ede3ef
Created attachment 339689 [details] [review] [PATCH] libnm: initialize @device_type of device objects How about the attached fix?
(In reply to Beniamino Galvani from comment #13) > Created attachment 339689 [details] [review] [review] > [PATCH] libnm: initialize @device_type of device objects > > How about the attached fix? if we now determine the device-type based on the present D-Bus interfaces, is that consistent with what the exposed device-type property? If we receive an unknown device-type, will we properly generate a generic device? NM_DEVICE_DEVICE_TYPE is READABLE only, there shouldn't be an entry in the set_property(). »···case PROP_DEVICE_TYPE: »···»···g_value_set_enum (value, nm_device_get_device_type (device)); after the patch, nm_device_get_device_type() can be basically any uint32. thus, get_property() must ensure to not return an invalid enum value, otherwise glib will raise a g_warning. »···case PROP_DEVICE_TYPE: »···»···g_value_set_enum (value, nm_device_get_device_type (device));
Created attachment 339781 [details] [review] [PATCH v2] libnm: initialize @device_type of device objects (In reply to Thomas Haller from comment #14) > (In reply to Beniamino Galvani from comment #13) > > Created attachment 339689 [details] [review] [review] [review] > > [PATCH] libnm: initialize @device_type of device objects > > > > How about the attached fix? > > if we now determine the device-type based on the present D-Bus interfaces, > is that consistent with what the exposed device-type property? Yes, as long as NM sends consistent information (where the device-type matches the interface). If this is not the case, it's a bug in NM. In theory it's possible that in the future we extend a device class and we add a new interface on top of it (for example we create a new macsec device based on ethernet) and so the D-Bus object would have both the known interface and the new one. With the current implementation we would create an ethernet device with type unknown, which doesn't sound wrong. > If we receive > an unknown device-type, will we properly generate a generic device? A NMDeviceGeneric requires the presence of the o.fd.*.Generic D-Bus interface, and thus we can't instantiate a generic device for unknown objects. > NM_DEVICE_DEVICE_TYPE is READABLE only, there shouldn't be an entry in the > set_property(). Fixed. > »···case PROP_DEVICE_TYPE: > »···»···g_value_set_enum (value, nm_device_get_device_type (device)); > > after the patch, nm_device_get_device_type() can be basically any uint32. > thus, get_property() must ensure to not return an invalid enum value, > otherwise glib will raise a g_warning. How about patch v2?
(In reply to Beniamino Galvani from comment #15) > Created attachment 339781 [details] [review] [review] > > »···case PROP_DEVICE_TYPE: > > »···»···g_value_set_enum (value, nm_device_get_device_type (device)); > > > > after the patch, nm_device_get_device_type() can be basically any uint32. > > thus, get_property() must ensure to not return an invalid enum value, > > otherwise glib will raise a g_warning. > > How about patch v2? (1) if your demarshal_device_type() is only for NMDeviceType, you can just as well implement it as NMDeviceType _coerce_valid (NMDeviceType device_type) { switch (device_type) { case NM_DEVICE_TYPE_ETHERNET: case NM_DEVICE_TYPE_WIFI: ... return device_type; case NM_DEVICE_TYPE_UNKNOWN: case NM_DEVICE_TYPE_UNUSED1: case NM_DEVICE_TYPE_UNUSED2: break; } return NM_DEVICE_TYPE_UNKNOWN; } advantage: it's faster, but still easy to understand. It's also easy to maintain, because you get a compiler warning when forgetting to handle a device-type. (2) on the other hand, if you implement it generically based on GType, demarshal_*() probably makes sense for other properties as well. It should be just called demarshal_valid_enum() and be considered for other properties. But maybe not, see (3). (3) Note that nm_device_get_device_type() and priv->device_type have no problems with out-of-range enum values. Only the gobject property has. So, instead of adding a demarshal_*() function that already coerces priv->device_type, you could instead only fix get_property(): g_value_set_enum (value, _coerce_valid (nm_device_get_device_type (device))); IMHO (3) and (1) is preferable.
(In reply to Thomas Haller from comment #16) > IMHO (3) and (1) is preferable. Changed and applied, thanks. https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=5dfc571971f749a35b5c6c22d73b38a99d10b840