GNOME Bugzilla – Bug 753566
Implement the ObjectManager interace and use it in libnm
Last modified: 2015-11-18 14:29:10 UTC
See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=773525 for more detail on the problem. TLDR: upstream D-Bus now has a default limit of 128 pending replies due to a security issue; we have been overriding that for years because NM exports many objects. But in the near future kdbus will have a hardcoded limit of 128 which we cannot change, and we'll run into this problem. http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager In that bug Simon (D-Bus maintainer) and I discuss solutions, and the one that I think is most workable is implementing the ObjectManager interface in the NM daemon. This will return *all* exported objects to the client in one D-Bus method call instead of one-call-per-object. Now that we have gdbus support, we can leverage GIO's ObjectManager implementation to do some of this. I've pushed some WIP patches to dcbw/gdbus-object-manager. There are a few issues: 1) The gdbus' ObjectManager implementation references objects, where previously we only weak-refed them. That means it's harder to track the object's life-cycle and un-export the object when its dead. We must remember to explicitly un-export objects when we are done with them, because just dropping the reference from the core code to eg an NMDevice doesn't drop the ObjectManager's reference, so the object lives on. Unexporting them ensures that the ObjectManager will also drop the reference and that the object will die. 2) We'll have to do some parkour around the ifcfg-rh plugin; gdbus doesn't like that its exported object has a different object path prefix than the rest of NetworkManager. But since that is not really a "public" API and it only exports one object, there must be a way to hack around it. This is the source of the "NULL" assertion message you see during NM startup. Next up would be converting libnm to use this interface when loading objects initially. Since we get the *entire* object tree of NetworkManager in one call, it should be pretty easy to create objects but instead of executing D-Bus calls to get child object properties, we just look up the relevant object by path in the GetManagedObjects() reply, and create it from that.
Pushed a few fixup commits. >> WIP: add support for DBus ObjectManager interface I guess the commit message is now outdated. Do you still see the issue with the singletons? Taking a reference (where needed) seems the right fix to me. Otherwise LGTM
Could you rebase again?
I squashed the commits and reworded the commit message. The original version is still at dcbw/gdbus-object-manager-1. Rebasing still todo.
Rebased on master. The code heavily changed. Please re-review.
There were several issues, most of them fixed now. For ifcfg-rh, I moved the entire service com.redhat.ifcfgrh1 to separete GDBusConnection. It seems to work now (actually even better, because now the service only expose /com/redhat/ifcfgrh1 object (and no longer all the /org/fd/NM). The only remaining issue that I see is that I get org.freedesktop.DBus.Error.AccessDenied when running as non-root. Strangle, a while ago I did not get that failure, must be related to some recent changes I did. Does somebody know more? Repushed. An older version of the branch is at dcbw/gdbus-object-manager-2. Please review and test.
> wifi/ap: explicitly unexport AP and refactor add/remove AP I think we should set the current AP after adding the AP to the list and emitting the add signal; otherwise you get the CurrentAP signal emitted before the AccessPointAdded signal, and the client might not yet have the current AP in their cache. I know this issue exists in the current code so you didn't add it, but I think we should fix it. I'm not sure why it was done this way originally. Also, ap_add_remove() is only called with set_current_ap=TRUE in one place (act_stage1_prepare()) so I think it would be clearer to just call set_current_ap() from act_stage1_prepare(). That also makes the code clearer since there's no way that add_remove_ap() could call set_current_ap() which could recursively call add_remove_ap() again. > ifcfg-rh: use distinct D-Bus connection for ifcfg-rh service Some whitespace issues in constructed(). > exported-object: add support for DBus ObjectManager interface Ugh, it seems that g_dbus_object_manager_get_object() returns a refed object. So nm-manager.c needs to make sure to unref that object in the "done:" section now too. Also I think we need to free 'path' in nm_dbus_manager_unregister_object() since we have to use g_object_get() to read it. Also should nm_exported_object_unexport() set priv->bus_mgr to NULL? I don't think removing the weak pointer does that (I could be wrong) but it not we'll still have a non-NULL priv->bus_mgr there.
(In reply to Dan Williams from comment #6) > > wifi/ap: explicitly unexport AP and refactor add/remove AP > > I think we should set the current AP after adding the AP to the list and > emitting the add signal; otherwise you get the CurrentAP signal emitted > before the AccessPointAdded signal, and the client might not yet have the > current AP in their cache. I know this issue exists in the current code so > you didn't add it, but I think we should fix it. I'm not sure why it was > done this way originally. I think both events were blocked by g_object_freeze_notify(). As I understand the documentation, it's not clear in which order the events will be thawed. Thus I moved the set_current_ap() *after* thaw_notify. > Also, ap_add_remove() is only called with set_current_ap=TRUE in one place > (act_stage1_prepare()) so I think it would be clearer to just call > set_current_ap() from act_stage1_prepare(). That also makes the code > clearer since there's no way that add_remove_ap() could call > set_current_ap() which could recursively call add_remove_ap() again. Done. > > ifcfg-rh: use distinct D-Bus connection for ifcfg-rh service > > Some whitespace issues in constructed(). fixed > > exported-object: add support for DBus ObjectManager interface > > Ugh, it seems that g_dbus_object_manager_get_object() returns a refed > object. So nm-manager.c needs to make sure to unref that object in the > "done:" section now too. Done. I prefer gs_unref_object here. > Also I think we need to free 'path' in nm_dbus_manager_unregister_object() > since we have to use g_object_get() to read it. That path was already gs_free. > Also should nm_exported_object_unexport() set priv->bus_mgr to NULL? I > don't think removing the weak pointer does that (I could be wrong) but it > not we'll still have a non-NULL priv->bus_mgr there. Definitely. Fixed all and repushed. Also new fixup commit to reconnect to ifcfgrh1 D-Bus service on SIGHUP and SIGUSR1. It felt a bit bad not to recover at all, so this gives the user a way to recover.
We can't export the objects to private socket connection and the regular dbus connection since their interface skeletons can only be paired with one particular connection and get unexported when a private connection closes. This branch removes the private socket; a commit message has some more justification for that: lr/gdbus-object-manager/no-private-socket > exported-object: add support for DBus ObjectManager interface > > +#define OBJECT_MANAGER_SERVER_BASE_PATH "/org/freedesktop" A nitpick: "/" probably makes more sense?
Also, we probably want this so that the unprivileged users can use OM: diff --git a/src/org.freedesktop.NetworkManager.conf b/src/org.freedesktop.NetworkManager.conf index e6f0306..c4082d2 100644 --- a/src/org.freedesktop.NetworkManager.conf +++ b/src/org.freedesktop.NetworkManager.conf @@ -37,6 +37,8 @@ send_interface="org.freedesktop.DBus.Introspectable"/> <allow send_destination="org.freedesktop.NetworkManager" send_interface="org.freedesktop.DBus.Properties"/> + <allow send_destination="org.freedesktop.NetworkManager" + send_interface="org.freedesktop.DBus.ObjectManager"/> <!-- Devices (read-only properties, no methods) --> <allow send_destination="org.freedesktop.NetworkManager"
(In reply to Lubomir Rintel from comment #8) > We can't export the objects to private socket connection and the regular > dbus connection since their interface skeletons can only be paired with one > particular connection and get unexported when a private connection closes. > This branch removes the private socket; a commit message has some more > justification for that: > > lr/gdbus-object-manager/no-private-socket I think we should do this. Let's merge the first part of dcbw/gdbus-object-manager except "exported-object: add support for DBus ObjectManager interface". Is that part ACK'ed? > > exported-object: add support for DBus ObjectManager interface > > > > +#define OBJECT_MANAGER_SERVER_BASE_PATH "/org/freedesktop" > > A nitpick: "/" probably makes more sense? why? (In reply to Lubomir Rintel from comment #9) > Also, we probably want this so that the unprivileged users can use OM: > > diff --git a/src/org.freedesktop.NetworkManager.conf > b/src/org.freedesktop.NetworkManager.conf > index e6f0306..c4082d2 100644 > --- a/src/org.freedesktop.NetworkManager.conf > +++ b/src/org.freedesktop.NetworkManager.conf > @@ -37,6 +37,8 @@ > > send_interface="org.freedesktop.DBus.Introspectable"/> > <allow send_destination="org.freedesktop.NetworkManager" > send_interface="org.freedesktop.DBus.Properties"/> > + <allow send_destination="org.freedesktop.NetworkManager" > + send_interface="org.freedesktop.DBus.ObjectManager"/> > > <!-- Devices (read-only properties, no methods) --> > <allow send_destination="org.freedesktop.NetworkManager" Fixed. Squashed fixup! commits and rebased.
(In reply to Thomas Haller from comment #10) > (In reply to Lubomir Rintel from comment #8) > > We can't export the objects to private socket connection and the regular > > dbus connection since their interface skeletons can only be paired with one > > particular connection and get unexported when a private connection closes. > > This branch removes the private socket; a commit message has some more > > justification for that: > > > > lr/gdbus-object-manager/no-private-socket > > I think we should do this. > > Let's merge the first part of dcbw/gdbus-object-manager except > "exported-object: add support for DBus ObjectManager interface". Is that > part ACK'ed? Yeah, LGTM. > > > exported-object: add support for DBus ObjectManager interface > > > > > > +#define OBJECT_MANAGER_SERVER_BASE_PATH "/org/freedesktop" > > > > A nitpick: "/" probably makes more sense? > > why? Seen BlueZ do that. the /org/freedesktop seems a bit arbitrary to me. It makes a bit more sense to me for the client to request "all objects owned by this bus client" rather than "objects owned by this bus client below /org/freedesktop".
(In reply to Lubomir Rintel from comment #11) > > > > exported-object: add support for DBus ObjectManager interface > > > > > > > > +#define OBJECT_MANAGER_SERVER_BASE_PATH "/org/freedesktop" > > > > > > A nitpick: "/" probably makes more sense? > > > > why? > > Seen BlueZ do that. the /org/freedesktop seems a bit arbitrary to me. It > makes a bit more sense to me for the client to request "all objects owned by > this bus client" rather than "objects owned by this bus client below > /org/freedesktop". did you actually test that? If you set the path to "/", GDBusObjectManager does: case PROP_OBJECT_PATH: g_assert (manager->priv->object_path == NULL); g_assert (g_variant_is_object_path (g_value_get_string (value))); manager->priv->object_path = g_value_dup_string (value); manager->priv->object_path_ending_in_slash = g_strdup_printf ("%s/", manager->priv->object_path); note that object_path_ending_in_slash is now "//". Later you'll see: g_return_if_fail (g_str_has_prefix (object_path, manager->priv->object_path_ending_in_slash)); Bluez doesn't use GDBusObjectManager.
(In reply to Lubomir Rintel from comment #8) > We can't export the objects to private socket connection and the regular > dbus connection since their interface skeletons can only be paired with one > particular connection and get unexported when a private connection closes. > This branch removes the private socket; a commit message has some more > justification for that: > > lr/gdbus-object-manager/no-private-socket kdbus went back to the drawing board, unfortunately. They're reworking the patches to be less like dbus and more general. I have no idea what's going to come out of that, and how much it's going to be like dbus, and whether existing apps can transparently support kdbus or not. In any case, if it's broken, then I guess we have to remove it. But are we 100% sure that it's broken? Anything running as root using libnm/libnm-glib should be using the private bus by default. Was it just that if dbus wasn't running, that NM wasn't doing the right thing even though the private socket stuff is OK?
merged early part of the branch to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=af10948e87d9a95ead17d35813a8e11be9c3eae0
(In reply to Dan Williams from comment #13) > (In reply to Lubomir Rintel from comment #8) > > We can't export the objects to private socket connection and the regular > > dbus connection since their interface skeletons can only be paired with one > > particular connection and get unexported when a private connection closes. > > This branch removes the private socket; a commit message has some more > > justification for that: > > > > lr/gdbus-object-manager/no-private-socket > > kdbus went back to the drawing board, unfortunately. They're reworking the > patches to be less like dbus and more general. I have no idea what's going > to come out of that, and how much it's going to be like dbus, and whether > existing apps can transparently support kdbus or not. > > In any case, if it's broken, then I guess we have to remove it. But are we > 100% sure that it's broken? Anything running as root using libnm/libnm-glib > should be using the private bus by default. Was it just that if dbus wasn't > running, that NM wasn't doing the right thing even though the private socket > stuff is OK? One main-usercase for the private socket was to start NM when no D-Bus is around yet. Precisely that was broken until http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=1c11c5cff15358026ea867a8127adb85173dfe5c , which means, nobody was actually was using that. Also, nobody was testing that (making it hard to get it right). Then, at several places we don't retry to connect to D-Bus (e.g. nm-auth-manager.c). That means, if you start without D-Bus running, NM will not recover from that once D-Bus appears. Also, stopping the D-Bus service is also entirely untested. That is something that NM can also not recover from. I'm in favor for removing it. Or alternatively, what are our options to fix ObjectManager with keeping the private socket?
I rebased lr/gdbus-object-manager/no-private-socket branch (previous version at lr/gdbus-object-manager/no-private-socket-1). Now we have lr/gdbus-object-manager/no-private-socket and dcbw/gdbus-object-manager and should decide how to proceed. Note, I also filed a bug 757945 to fix object-manager.
I merged lr/gdbus-object-manager/no-private-socket to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=bc5a8e2e180c2d51189d852c8bcd3ee66e3b1dc0 With this, object-manager is in place and the private-socket is dropped. What is still missing, is to make use of the ObjectManager from libnm.