GNOME Bugzilla – Bug 754794
[PATCH] libnm: don't add objects to cache until they're async-inited
Last modified: 2015-10-06 13:18:03 UTC
Created attachment 311014 [details] libnm: don't add objects to cache until they're async-inited Otherwise the uninitializeded objects could be prematurely signalled if their paths are seen twice in quick succession. This happens when you have ethernet hardware and add an ethernet connection -- it's immediatelly added to AvialableConnections and the property reload signals the object addition before the NMRemoteSettings's GetSettings() finishes: # nmcli c add type ethernet autoconnect no ifname '*' (process:4610): libnm-CRITICAL **: nm_connection_get_id: assertion 's_con != NULL' failed Connection '(null)' ((null)) successfully added. #
this is a duplicate of bug 754767
*** Bug 754767 has been marked as a duplicate of this bug. ***
patch merged to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=88f0d646d5089dfc8df9b03750ade72e921803eb But it breaks `make check -C libnm`... REOPEN
(In reply to Thomas Haller from comment #3) > patch merged to master as > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/ > ?id=88f0d646d5089dfc8df9b03750ade72e921803eb > > > But it breaks `make check -C libnm`... REOPEN reverted too: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0e5af7fa46c2363315eda0963c1826b8ca9d6a53
Created attachment 312178 [details] A new fix
(In reply to Lubomir Rintel from comment #5) > Created attachment 312178 [details] > A new fix make check -C libnm/ still fails here...
Created attachment 312321 [details] A new fix (In reply to Thomas Haller from comment #6) > (In reply to Lubomir Rintel from comment #5) > > Created attachment 312178 [details] > > A new fix > > make check -C libnm/ > > still fails here... Whoops, sorry. Works now.
The test passes now. I wonder, returning CONTINUE from deferred_notify_cb() effectively means busy-waiting. I think you should return G_SOURCE_REMOVE and always clear the notify-id... I just don't understand from the code, whether that would mean we miss a notification.
Created attachment 312682 [details] A fixed version of the fix for a fix (In reply to Thomas Haller from comment #8) > The test passes now. > > I wonder, returning CONTINUE from deferred_notify_cb() effectively means > busy-waiting. I think you should return G_SOURCE_REMOVE and always clear the > notify-id... Yes, you're right. I assumed the main loop doesn't spin when there's only idle sources (which is why I owe you a beer). It was probably that the dbus response was really quick to run when I was looking so that it didn't look like a busy loop (if the nm-remote-settings' GetSettings call took a bit longer then the client would indeed busy-wait here). > I just don't understand from the code, whether that would mean we miss a > notification. Yes, we'd miss a notification. Added a list of objects whose properties are pending notifications that should be emitted after the init. Can't see a cleaner solution. At least the tests run, and I've confirmed it to fix the bug.
Created attachment 312684 [details] [review] fixup! libnm: avoid notifying for objects until they're async-inited The item (waiter) has a strong reference to the waited-items. But I would not take a strong reference in the other direction too.
This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.