1. 24 Apr, 2018 1 commit
    • Thomas Haller's avatar
      core/dbus: stop NMDBusManager and reject future method calls · 3d2da8cd
      Thomas Haller authored
      During shutdown, we will need to still iterate the main loop
      to do a coordinated shutdown. Currently we do not, and we just
      exit, leaving a lot of objects hanging.
      If we are going to fix that, we need during shutdown tell
      NMDBusManager to reject all future operations.
      Note that property getters and "GetManagerObjects" call is not
      blocked. It continues to work.
      Certainly for some operations, we want to allow them to be called even
      during shutdown. However, these have to opt-in.
      This also fixes an uglyness, where nm_dbus_manager_start() would
      get the set-property-handler and the @manager as user-data. However,
      NMDBusManager will always outlife NMManager, hence, after NMManager
      is destroyed, the user-data would be a dangling pointer. Currently
      that is not an issue, because
        - we always leak NMManager
        - we don't run the mainloop during shutdown
  2. 19 Apr, 2018 1 commit
    • Beniamino Galvani's avatar
      core: unexport dbus-objects on dispose · 21d3f168
      Beniamino Galvani authored
      When the D-Bus name is already taken, NM crashes in the following
      way. That's because disposed object are not unexported when quitting
      and so they linger in the bus-manager's list of exported objects,
      causing an invalid access when a neighboring item is accessed. Instead
      of just clearing the path, fully unexport the object.
      The behavior of not forcefully exporting objects on quit was added in
      f9ee20a7 ("core: explicitly unexport objects when we're done with
      them"), but such behavior doesn't seem to be needed by the stated
       <error> [1524062008.1886] bus-manager: fatal failure to acquire D-Bus service "org.freedesktop.NetworkManager" (3). Service already taken
       <trace> [1524062008.2327] config: state: success writing state file "/var/lib/NetworkManager/NetworkManager.state"
       <trace> [1524062008.2338] dns-mgr: stopping...
       <info>  [1524062008.2344] exiting (error)
       <debug> [1524062008.2628] disposing NMManager singleton (0xce587e0)
       <trace> [1524062008.2640] dns-mgr: disposing
       <debug> [1524062008.2651] disposing NMDnsManager singleton (0xceb8b50)
       <debug> [1524062008.2666] disposing NMFirewallManager singleton (0xceb62b0)
       <debug> [1524062008.2709] disposing NMHostnameManager singleton (0xce7b370)
       <trace> [1524062008.2722] dbus-object[0xce70f40]: unexport: "/org/freedesktop/NetworkManager/AgentManager"
       ==16381== Invalid write of size 8
       ==16381==    at 0x42F511: c_list_unlink_stale (c-list.h:158)
       ==16381==    by 0x42F511: c_list_unlink (c-list.h:171)
       ==16381==    by 0x42F511: _nm_dbus_manager_obj_unexport (nm-dbus-manager.c:1135)
       ==16381==    by 0x4C5E35: nm_dbus_object_unexport (nm-dbus-object.c:165)
       ==16381==    by 0x5C01E9: dispose (nm-agent-manager.c:1634)
       ==16381==    by 0x6636F37: g_object_unref (gobject.c:3303)
       ==16381==    by 0x4BDC89: _nm_singleton_instance_destroy (nm-core-utils.c:138)
       ==16381==    by 0x400FA85: _dl_fini (in /usr/lib64/ld-2.27.so)
       ==16381==    by 0x7F806AB: __run_exit_handlers (in /usr/lib64/libc-2.27.so)
       ==16381==    by 0x7F807DB: exit (in /usr/lib64/libc-2.27.so)
       ==16381==    by 0x41DA34: main (main.c:463)
       ==16381==  Address 0xce706a0 is 48 bytes inside a block of size 176 free'd
       ==16381==    at 0x4C2EDAC: free (vg_replace_malloc.c:530)
       ==16381==    by 0x6ACA3E1: g_free (gmem.c:194)
       ==16381==    by 0x6AE2572: g_slice_free1 (gslice.c:1136)
       ==16381==    by 0x66550AE: g_type_free_instance (gtype.c:1943)
       ==16381==    by 0x4505F8: dispose (nm-manager.c:6867)
       ==16381==    by 0x6636F37: g_object_unref (gobject.c:3303)
       ==16381==    by 0x4BDC89: _nm_singleton_instance_destroy (nm-core-utils.c:138)
       ==16381==    by 0x400FA85: _dl_fini (in /usr/lib64/ld-2.27.so)
       ==16381==    by 0x7F806AB: __run_exit_handlers (in /usr/lib64/libc-2.27.so)
       ==16381==    by 0x7F807DB: exit (in /usr/lib64/libc-2.27.so)
       ==16381==    by 0x41DA34: main (main.c:463)
       ==16381==  Block was alloc'd at
       ==16381==    at 0x4C2DBAB: malloc (vg_replace_malloc.c:299)
       ==16381==    by 0x6ACA2D5: g_malloc (gmem.c:99)
       ==16381==    by 0x6AE1E36: g_slice_alloc (gslice.c:1025)
       ==16381==    by 0x6AE247C: g_slice_alloc0 (gslice.c:1051)
       ==16381==    by 0x6654E09: g_type_create_instance (gtype.c:1848)
       ==16381==    by 0x66376C7: g_object_new_internal (gobject.c:1799)
       ==16381==    by 0x6638E14: g_object_new_with_properties (gobject.c:1967)
       ==16381==    by 0x66399D0: g_object_new (gobject.c:1639)
       ==16381==    by 0x5D6F18: nm_settings_new (nm-settings.c:1897)
       ==16381==    by 0x4514B4: constructed (nm-manager.c:6489)
       ==16381==    by 0x66378FA: g_object_new_internal (gobject.c:1839)
       ==16381==    by 0x6638E14: g_object_new_with_properties (gobject.c:1967)
  3. 13 Apr, 2018 1 commit
    • Thomas Haller's avatar
      core: convert NMDBusObject's "path" property to signal "exported-changed" · cd71ec30
      Thomas Haller authored
      The GObject property "path" exists for the sole reasons so that
      other components can connect to the "notify::path" signal.
      However, notifications are blocked by g_object_freeze_notify(),
      and especially for NMDBusObject we want to make use of that to
      combine multiple PropertiesChanged events into one.
      This blocking of the signal is not desired for the case where
      we wait for "notify::path". Convert that to a signal, which
      will not be blocked.
  4. 13 Mar, 2018 1 commit
    • Thomas Haller's avatar
      core/dbus: rework creating numbered D-Bus export path by putting counter into class · 57ab9fd6
      Thomas Haller authored
      I dislike the static hash table to cache the integer counter for
      numbered paths. Let's instead cache the counter at the class instance
      itself -- since the class contains the information how the export
      path should be exported.
      However, we cannot use a plain integer field inside the class structure,
      because the class is copied between derived classes. For example,
      NMDeviceEthernet and NMDeviceBridge both get a copy of the NMDeviceClass
      instance. Hence, the class doesn't contain the counter directly, but
      a pointer to one counter that can be shared between sibling classes.
  5. 12 Mar, 2018 1 commit
    • Thomas Haller's avatar
      core/dbus: rework D-Bus implementation to use lower layer GDBusConnection API · 297d4985
      Thomas Haller authored
      Previously, we used the generated GDBusInterfaceSkeleton types and glued
      them via the NMExportedObject base class to our NM types. We also used
      Don't do that anymore. The resulting code was more complicated despite (or
      because?) using generated classes. It was hard to understand, complex, had
      ordering-issues, and had a runtime and memory overhead.
      This patch refactors this entirely and uses the lower layer API GDBusConnection
      directly. It replaces the generated code, GDBusInterfaceSkeleton, and
      GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
      and static descriptor instances of type GDBusInterfaceInfo.
      This adds a net plus of more then 1300 lines of hand written code. I claim
      that this implementation is easier to understand. Note that previously we
      also required extensive and complex glue code to bind our objects to the
      generated skeleton objects. Instead, now glue our objects directly to
      GDBusConnection. The result is more immediate and gets rid of layers of
      code in between.
      Now that the D-Bus glue us more under our control, we can address issus and
      bottlenecks better, instead of adding code to bend the generated skeletons
      to our needs.
      Note that the current implementation now only supports one D-Bus connection.
      That was effectively the case already, although there were places (and still are)
      where the code pretends it could also support connections from a private socket.
      We dropped private socket support mainly because it was unused, untested and
      buggy, but also because GDBusObjectManagerServer could not export the same
      objects on multiple connections. Now, it would be rather straight forward to
      fix that and re-introduce ObjectManager on each private connection. But this
      commit doesn't do that yet, and the new code intentionally supports only one
      D-Bus connection.
      Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
      succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
      connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
      for the moment. It could be easily extended later, for example with polling whether the
      system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
      supported either -- just like before.
      Note how NMDBusManager now implements the ObjectManager D-Bus interface
      Also, this fixes race issues in the server, by no longer delaying
      PropertiesChanged signals. NMExportedObject would collect changed
      properties and send the signal out in idle_emit_properties_changed()
      on idle. This messes up the ordering of change events w.r.t. other
      signals and events on the bus. Note that not only NMExportedObject
      messed up the ordering. Also the generated code would hook into
      notify() and process change events in and idle handle, exhibiting the
      same ordering issue too.
      No longer do that. PropertiesChanged signals will be sent right away
      by hooking into dispatch_properties_changed(). This means, changing
      a property in quick succession will no longer be combined and is
      guaranteed to emit signals for each individual state. Quite possibly
      we emit now more PropertiesChanged signals then before.
      However, we are now able to group a set of changes by using standard
      g_object_freeze_notify()/g_object_thaw_notify(). We probably should
      make more use of that.
      Also, now that our signals are all handled in the right order, we
      might find places where we still emit them in the wrong order. But that
      is then due to the order in which our GObjects emit signals, not due
      to an ill behavior of the D-Bus glue. Possibly we need to identify
      such ordering issues and fix them.
      Numbers (for contrib/rpm --without debug on x86_64):
      - the patch changes the code size of NetworkManager by
        - 2809360 bytes
        + 2537528 bytes (-9.7%)
      - Runtime measurements are harder because there is a large variance
        during testing. In other words, the numbers are not reproducible.
        Currently, the implementation performs no caching of GVariants at all,
        but it would be rather simple to add it, if that turns out to be
        Anyway, without strong claim, it seems that the new form tends to
        perform slightly better. That would be no surprise.
        $ time (for i in {1..1000}; do nmcli >/dev/null || break; echo -n .;  done)
        - real    1m39.355s
        + real    1m37.432s
        $ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects > /dev/null || break; echo -n .; done)
        - real    0m26.843s
        + real    0m25.281s
      - Regarding RSS size, just looking at the processes in similar
        conditions, doesn't give a large difference. On my system they
        consume about 19MB RSS. It seems that the new version has a
        slightly smaller RSS size.
        - 19356 RSS
        + 18660 RSS