1. 15 Mar, 2018 3 commits
  2. 14 Mar, 2018 1 commit
  3. 13 Mar, 2018 2 commits
    • Beniamino Galvani's avatar
      dhcp: handle expiry by letting the client continue for some time · 17009ed9
      Beniamino Galvani authored
      Previously we would kill the client when the lease expired and we
      restarted it 3 times at 2 minutes intervals before failing the
      connection. If the client is killed after it received a NACK from the
      server, it doesn't have the chance to delete the lease file and the
      next time it is started it will request the same lease again.
      
      Also, the previous restart logic is a bit convoluted.
      
      Since clients already know how to deal with NACKs, let them continue
      for a grace period after the expiry. When the grace period ends, we
      fail the method and this can either fail the whole connection or keep
      it active depending on the may-fail configuration.
      
      https://bugzilla.gnome.org/show_bug.cgi?id=783391
      17009ed9
    • 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.
      57ab9fd6
  4. 12 Mar, 2018 7 commits
    • Thomas Haller's avatar
    • 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
      GDBusObjectManagerServer.
      
      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
      directly.
      
      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
        useful.
        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
      297d4985
    • Thomas Haller's avatar
      core: rename "nm-bus-manager.h" to "nm-dbus-manager.h" · a1f37964
      Thomas Haller authored
      The next commit will completely rework NMBusManager and replace
      NMExportedObject by a new type NMDBusObject.
      
      Originally, NMDBusObject was added along NMExportedObject to ease
      the rework and have compilable, intermediate stages of refactoring. Now,
      I think the new name is better, because NMDBusObject is very strongly related
      to the bus manager and the old name NMExportedObject didn't make that
      clear.
      
      I also slighly prefer the name NMDBusObject over NMBusObject, hence
      for consistancy, also rename NMBusManager to NMDBusManager.
      
      This commit only renames the file for a nicer diff in the next commit.
      It does not actually update the type name in sources. That will be done
      later.
      a1f37964
    • Thomas Haller's avatar
      device: set properties before emitting the change notification · 8b75f10e
      Thomas Haller authored
      The change doesn't really make a difference. I thought it would, so I
      did it. But turns out (as the code correctly assumes), while the
      notifications are frozen, it's OK to leave the property still in an
      inconsistent state while emitting the notify signal.
      
      Still, it feels slightly more correct this way, so keep the change.
      8b75f10e
    • Thomas Haller's avatar
      device/veth: don't use notify() signal to bind changes for "peer" property · 34493c51
      Thomas Haller authored
      The notify() signal is not emitted while the object properties are
      blocked via g_object_freeze_notify(). That makes is unsuitable to
      emit a notification for "peer" property whenver the device's "parent"
      property changes. Because especially with freeze/thaw, we want to emit
      both signals in the same batch, not first emit change signals for "parent",
      and then in a second run the signals for "peer".
      
      Use the existing parent_changed_notify() virtual function instead.
      34493c51
    • Thomas Haller's avatar
      secret-agent: don't use generated NMDBusSecretAgent proxy · 062f86d8
      Thomas Haller authored
      The generated code is really just a thin wrapper around direct
      GDBusProxy calls. GDBusProxy is reasonably convenient to use directly,
      drop this wrapper.
      
      We also don't use a generated wrapper for other cases where
      NetworkManager acts as a D-Bus client. There is no reason to
      do it in this case.
      
      While the nmdbus_*() functions that we were using are small wrappers,
      we also created a NMDBusSecretAgent instance, and hence several other
      functions and symbols are used as well. It's unnecessary.
      
      This saves 8552 bytes for NetworkManager binary (2817944 vs. 2809392
      bytes for contrib/rpm on x86_64).
      062f86d8
    • Thomas Haller's avatar
  5. 10 Mar, 2018 4 commits
    • Thomas Haller's avatar
      b0b5cfb7
    • Thomas Haller's avatar
      14ffe6bc
    • Thomas Haller's avatar
      dns: don't leak cached config_variant on exit · 70b24819
      Thomas Haller authored
      Not really serious as it's only on shutdown, but shows up as leak in valgrind.
      70b24819
    • Thomas Haller's avatar
      manager: fix leaking volatile-connection-list on exit · 0b756184
      Thomas Haller authored
      On exit during NMManager's dispose(), we must fist remove active connections
      via active_connection_remove(), before clearing the volatile-connection-list.
      Otheriwise, while deleting the active connection, we schedule a idle action
      to delete the volatile connection on idle, but at that point the dispose()
      already cleaned up the idle list.
      
        ==3150== 72 (24 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 3,411 of 6,079
        ==3150==    at 0x4C2FB6B: malloc (vg_replace_malloc.c:299)
        ==3150==    by 0x6AB7358: g_malloc (gmem.c:94)
        ==3150==    by 0x6ACEF35: g_slice_alloc (gslice.c:1025)
        ==3150==    by 0x1686B1: connection_flags_changed (nm-manager.c:1823)
        ==3150==    by 0x661F73C: g_closure_invoke (gclosure.c:804)
        ==3150==    by 0x66324DD: signal_emit_unlocked_R (gsignal.c:3635)
        ==3150==    by 0x663AD04: g_signal_emit_valist (gsignal.c:3391)
        ==3150==    by 0x663B66E: g_signal_emit (gsignal.c:3447)
        ==3150==    by 0x2EC753: connection_flags_changed (nm-settings.c:824)
        ==3150==    by 0x661F73C: g_closure_invoke (gclosure.c:804)
        ==3150==    by 0x66324DD: signal_emit_unlocked_R (gsignal.c:3635)
        ==3150==    by 0x663AD04: g_signal_emit_valist (gsignal.c:3391)
        ==3150==    by 0x663B66E: g_signal_emit (gsignal.c:3447)
        ==3150==    by 0x6623C03: g_object_dispatch_properties_changed (gobject.c:1080)
        ==3150==    by 0x1DFD47: dispatch_properties_changed (nm-dbus-object.c:237)
        ==3150==    by 0x6626178: g_object_notify_by_spec_internal (gobject.c:1173)
        ==3150==    by 0x6626178: g_object_notify_by_pspec (gobject.c:1283)
        ==3150==    by 0x2E7377: _notify (nm-settings-connection.c:53)
        ==3150==    by 0x2E7377: nm_settings_connection_set_flags_full (nm-settings-connection.c:2346)
        ==3150==    by 0x2E744D: nm_settings_connection_set_flags (nm-settings-connection.c:2316)
        ==3150==    by 0x2E7466: set_visible (nm-settings-connection.c:316)
        ==3150==    by 0x2E7774: nm_settings_connection_delete (nm-settings-connection.c:795)
        ==3150==    by 0x1665A8: _delete_volatile_connection_do (nm-manager.c:598)
        ==3150==    by 0x1668F4: active_connection_remove (nm-manager.c:625)
        ==3150==    by 0x16ABA7: dispose (nm-manager.c:6726)
        ==3150==    by 0x6624607: g_object_unref (gobject.c:3293)
        ==3150==    by 0x1D779B: _nm_singleton_instance_destroy (nm-core-utils.c:138)
        ==3150==    by 0x4011332: _dl_fini (in /usr/lib64/ld-2.26.so)
        ==3150==    by 0x815FB57: __run_exit_handlers (in /usr/lib64/libc-2.26.so)
        ==3150==    by 0x815FBA9: exit (in /usr/lib64/libc-2.26.so)
        ==3150==    by 0x1383C7: main (main.c:467)
      0b756184
  6. 09 Mar, 2018 6 commits
  7. 08 Mar, 2018 4 commits
  8. 07 Mar, 2018 1 commit
  9. 06 Mar, 2018 1 commit
  10. 05 Mar, 2018 3 commits
  11. 04 Mar, 2018 5 commits
    • Andrew Zaborowski's avatar
      iwd: don't call nm_wifi_ap_set_ssid for empty SSID · 29e9d206
      Andrew Zaborowski authored
      If SSID is an empty string there's no need to call nm_wifi_ap_set_ssid
      as it won't do anything.  It also has an assert checking that NULL is
      passed for an empty SSID and we were passing a non-NULL pointer.
      29e9d206
    • Andrew Zaborowski's avatar
      iwd: fix device-added signal handler signature · 8435aa8b
      Andrew Zaborowski authored
      This bug was not causing a crash for me because of the !IS_NM_DEVICE_IWD
      check and because my glib version probably had the assertion within
      NM_IWD_MANAGER_GET_PRIVATE disabled.
      
      While there, change the g_signal_connect line to use the macro for the
      signal name.
      8435aa8b
    • Andrew Zaborowski's avatar
      iwd: set Device.Powered during set_enable · 6571b576
      Andrew Zaborowski authored
      Make sure .set_enabled uses the Device.Powered property to basically
      bring the netdev UP and DOWN as I understand is expected by the
      nm_device logic.
      
      Device.Powered should generally reflect the UP state immediately but
      just to avoid possible race conditions .is_available() will now return
      a value that is an AND of the local "enabled" state and IWD's Powered
      property.
      6571b576
    • Andrew Zaborowski's avatar
      iwd: Disable timeout for iwd Device.Connect call · f1726810
      Andrew Zaborowski authored
      Change from the default dbus call timeout (-1) to infinite (G_MAXINT)
      because the call may now include the secret requests which have their
      own timeout policies.
      f1726810
    • Andrew Zaborowski's avatar
      iwd: Only request secrets on request from IWD · 90075179
      Andrew Zaborowski authored
      Remove the code (mostly copied from nm-device-wifi.c) that handles
      checking if the secrets were provided and requesting missing secrets
      before starting a connection attempt.  Instead, request secrets when
      we're asked for them by IWD through its agent interface.  This happens
      while the dbus Connect call is running.  We change the NMDevice from
      the CONFIG state to NEED_AUTH and then change back to CONFIG once we
      sent the secrets back to IWD.
      
      The current code would require the secrets only based on whether a
      network is a KnownNetwork but IWD may need a new passwords even for
      KnownNetworks if the last connection attempt has failed.
      90075179
  12. 03 Mar, 2018 3 commits