1. 04 Apr, 2018 11 commits
    • Thomas Haller's avatar
      checkpoint: allow overlapping checkpoints · 5c283356
      Thomas Haller authored
      that allows the creation of overlapping checkpoints. Before, and
      by default, checkpoints that reference a same device conflict,
      and creating such a checkpoint failed.
      Now, allow this. But during rollback automatically destroy all
      overlapping checkpoints that were created after the checkpoint
      that is about to rollback.
      With this, you can create a series of checkpoints, and rollback them
      individually. With the restriction, that if you once rolled back to an
      older checkpoint, you no longer can roll"forward" to a younger one.
      What this implies and what is new here, is that the checkpoint might be
      automatically destroyed by NetworkManager before the timeout expires. When
      the user later would try to manually destroy/rollback such a checkpoint, it
      would fail because the checkpoint no longer exists.
    • Thomas Haller's avatar
      checkpoint: don't let nm_checkpoint_new() fail · e5fc9a30
      Thomas Haller authored
      We already do error checking in nm_checkpoint_manager_create(). No need
      to split it in two places. Let all error conditions be handled by
      nm_checkpoint_manager_create() first, and then once we decide all is
      good, nm_checkpoint_new() can no longer fail.
    • Thomas Haller's avatar
      checkpoint: let each checkpoint schedule its own timeout · 5fb65b7f
      Thomas Haller authored
      Instead of scheduling one timeout only, let each checkpoint instance
      individually schedule a timeout. This has some overhead, but glib
      is supposed to make scheduling many timers efficient. Otherwise,
      glib should be fixed.
      This simplifies in my opinion the code, because it's up to each
      checkpoint to maintain its own timeout.
      Later we will also add a AdjustRollbackTimeout operation, which
      allow to reschedule the timeout. It also seems slightly simpler,
      if scheduling of the timeout is done by the NMCheckpoint instance
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      checkpoint: don't explicitly track checkpoints in a GHashTable · 79458a55
      Thomas Haller authored
      We already have a GHashTable for exported objects. We can use
      that if we want to look up by path efficiently.
    • Thomas Haller's avatar
      checkpoint: refactor nm_checkpoint_manager_create() to simplify creating device list · 63e3bff9
      Thomas Haller authored
      If no device paths are given, we can take the devices directly.
      We don't need to first create a list of paths, and then
      look them up by path again to add them to the list.
    • Thomas Haller's avatar
      checkpoint: skip unrealized devices in nm_checkpoint_manager_create() · daf3d5cb
      Thomas Haller authored
      We already do it for the case where no paths are provided.
    • Thomas Haller's avatar
      checkpoint/trivial: rename local variable @checkpoint_path · 6d6b3890
      Thomas Haller authored
      path is long enough and (in this context) it consistently
      references the checkpoint "path".
    • Thomas Haller's avatar
      checkpoint/trivial: rename nm_checkpoint_manager_unref() to nm_checkpoint_manager_free() · 45c24fb9
      Thomas Haller authored
      NMCheckpointManager was added and is not ref-countable, because it
      is not needed.
      I still often like for such objects (that are not ref-countable),
      that their destroy function is called "unref". Both for consistency,
      and also if we would later add ref-counting to the object.
      However, NMCheckpointManager keeps a pointer to NMManager. So, when
      NMManager gets destroyed, it *MUST* destroy the NMCheckpointManager.
      It cannot accept that the checkpoint manager outlives NMManager,
      but the "unref" name suggests that somebody else might have still
      a reference to this object keeping it alive. That is not the case.
      Rename so that this is clear.
      I would name it nm_checkpoint_manager_destroy(), but "destroy" already
      has a meaning for NMCheckpoint instances, so use "free".
    • Thomas Haller's avatar
      checkpoint: embed CList in NMCheckpoint instance · ffb49267
      Thomas Haller authored
      We don't need an external CheckpointItem, just to wrap the
      CList instance. Embed it directly in NMCheckpoint.
      Sure, that exposes the checkpoints_lst field in the (internal)
      header file, hiding the private member less.
    • Thomas Haller's avatar
      core: add macro for iterating CList of devices of NMManager · 8de522fa
      Thomas Haller authored
      I find it slightly nicer and explict. Also, the list elements
      are strictly speaking private, we should better not explicitly
      use them outside of NMManager/NMDevice. The macro hides this.
  2. 27 Mar, 2018 1 commit
    • Thomas Haller's avatar
      core: track devices in manager via embedded CList · 4a705e1a
      Thomas Haller authored
      Instead of using a GSList for tracking the devices, use a CList.
      I think a CList is in most cases the more suitable data structure
      then GSList:
       - you can find out in O(1) whether the object is linked. That
         is nice, for example to assert in NMDevice's destructor that
         the object was unlinked, and we will use that later in
       - you can unlink the element in O(1) and you can unlink the
         element without having access to the link's head
       - Contrary to GSList, this does not require an extra slice
         allocation for the link node. It quite possibliy consumes
         slightly less memory because the CList structure is embedded
         in a struct that we already allocate. Even if slice allocation
         would be perfect to only consume 2*sizeof(gpointer) for the link
         note, it would at most be as-good as CList. Quite possibly,
         there is an overhead though.
       - CList possibly has better memory locality, because the link
         structure and the data are close to each other.
      Something which could be seen as disavantage, is that with CList
      one device can only be tracked in one NMManager instance at a time.
      But that is fine. There exists only one NMManager instance for now,
      and even if we would ever introduce multiple managers, we probably
      would not associate one NMDevice instance with multiple managers.
      The advantages are arguably not huge, but CList is IMHO clearly the
      more suited data structure. No need to stick to a suboptimal data
      structure for the job. Refactor it.
  3. 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
  4. 18 Jan, 2018 1 commit
  5. 28 Nov, 2017 1 commit
    • Thomas Haller's avatar
      c-list: re-import latest version of c-list.h from upstream · b6efac9e
      Thomas Haller authored
      Most notably, it renames
        c_list_unlink_init() -> c_list_unlink()
        c_list_unlink() -> c_list_unlink_stale()
        $ sed -e 's/\<c_list_unlink\>/c_list_unlink_old/g' \
              -e 's/\<c_list_unlink_init\>/c_list_unlink/g' \
              -e 's/\<c_list_unlink_old\>/c_list_unlink_stale/g' \
              $(git grep -l c_list_unlink -- ':(exclude)shared/nm-utils/c-list.h') \
  6. 09 Nov, 2017 6 commits
  7. 18 Oct, 2017 1 commit
    • Thomas Haller's avatar
      core,clients: use our own string hashing function nm_str_hash() · 34342618
      Thomas Haller authored
      Replace the usage of g_str_hash() with our own nm_str_hash().
      GLib's g_str_hash() uses djb2 hashing function, just like we
      do at the moment. The only difference is, that we use a diffrent
      seed value.
      Note, that we initialize the hash seed with random data (by calling
      getrandom() or reading /dev/urandom). That is a change compared to
      This change of the hashing function and accessing the random pool
      might be undesired for libnm/libnm-core. Hence, the change is not
      done there as it possibly changes behavior for public API. Maybe
      we should do that later though.
      At this point, there isn't much of a change. This patch becomes
      interesting, if we decide to use a different hashing algorithm.
  8. 21 Nov, 2016 1 commit
    • Thomas Haller's avatar
      build: don't add subdirectories to include search path but require qualified include · 44ecb415
      Thomas Haller authored
      Keep the include paths clean and separate. We use directories to group source
      files together. That makes sense (I guess), but then we should use this
      grouping also when including files. Thus require to #include files with their
      path relative to "src/".
      Also, we build various artifacts from the "src/" tree. Instead of having
      individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
      unified. Previously, the CFLAGS for each artifact differ and are inconsistent
      in which paths they add to the search path. Fix the inconsistency by just
      don't add the paths at all.
  9. 24 Oct, 2016 1 commit
    • Beniamino Galvani's avatar
      checkpoint: introduce new flags to better restore previous state · ddeef464
      Beniamino Galvani authored
      When a global checkpoint is created (one with empty device list) we
      save the status of all devices to restore it later. After the
      checkpoint new interfaces and connections may appear and they can
      significantly influence the overall networking status, but we don't
      consider them at the moment.
      Introduce a new flag DELETE_NEW_CONNECTIONS to delete any connection
      added after the checkpoint and similarly a DISCONNECT_NEW_DEVICES to
      ensure that the connection active on newly appeared devices doesn't
      disrupt network connectivity.
  10. 14 Oct, 2016 1 commit
  11. 04 Oct, 2016 1 commit
    • Thomas Haller's avatar
      core: refactor private data in "src" · 4d37f7a1
      Thomas Haller authored
      - use _NM_GET_PRIVATE() and _NM_GET_PRIVATE_PTR() everywhere.
      - reorder statements, to have GObject related functions (init, dispose,
        constructed) at the bottom of each file and in a consistent order w.r.t.
        each other.
      - unify whitespaces in signal and properties declarations.
      - use NM_GOBJECT_PROPERTIES_DEFINE() and _notify()
      - drop unused signal slots in class structures
      - drop unused header files for device factories
  12. 26 Sep, 2016 1 commit
  13. 17 Aug, 2016 1 commit