1. 16 Jul, 2019 1 commit
    • Thomas Haller's avatar
      settings: rework tracking settings connections and settings plugins · d35d3c46
      Thomas Haller authored
      Completely rework how settings plugin handle connections and how
      NMSettings tracks the list of connections.
      
      Previously, settings plugins would return objects of (a subtype of) type
      NMSettingsConnection. The NMSettingsConnection was tightly coupled with
      the settings plugin. That has a lot of downsides.
      
      Change that. When changing this basic relation how settings connections
      are tracked, everything falls appart. That's why this is a huge change.
      Also, since I have to largely rewrite the settings plugins, I also
      added support for multiple keyfile directories, handle in-memory
      connections only by keyfile plugin and (partly) use copy-on-write NMConnection
      instances. I don't want to spend effort rewriting large parts while
      preserving the old way, that anyway should change. E.g. while rewriting ifcfg-rh,
      I don't want to let it handle in-memory connections because that's not right
      long-term.
      
      --
      
      If the settings plugins themself create subtypes of NMSettingsConnection
      instances, then a lot of knowledge about tracking connections moves
      to the plugins.
      Just try to follow the code what happend during nm_settings_add_connection().
      Note how the logic is spread out:
       - nm_settings_add_connection() calls plugin's add_connection()
       - add_connection() creates a NMSettingsConnection subtype
       - the plugin has to know that it's called during add-connection and
         not emit NM_SETTINGS_PLUGIN_CONNECTION_ADDED signal
       - NMSettings calls claim_connection() which hocks up the new
         NMSettingsConnection instance and configures the instance
         (like calling nm_settings_connection_added()).
      This summary does not sound like a lot, but try to follow that code. The logic
      is all over the place.
      
      Instead, settings plugins should have a very simple API for adding, modifying,
      deleting, loading and reloading connections. All the plugin does is to return a
      NMSettingsStorage handle. The storage instance is a handle to identify a profile
      in storage (e.g. a particular file). The settings plugin is free to subtype
      NMSettingsStorage, but it's not necessary.
      There are no more events raised, and the settings plugin implements the small
      API in a straightforward manner.
      NMSettings now drives all of this. Even NMSettingsConnection has now
      very little concern about how it's tracked and delegates only to NMSettings.
      
      This should make settings plugins simpler. Currently settings plugins
      are so cumbersome to implement, that we avoid having them. It should not be
      like that and it should be easy, beneficial and lightweight to create a new
      settings plugin.
      
      Note also how the settings plugins no longer care about duplicate UUIDs.
      Duplicated UUIDs are a fact of life and NMSettings must handle them. No
      need to overly concern settings plugins with that.
      
      --
      
      NMSettingsConnection is exposed directly on D-Bus (being a subtype of
      NMDBusObject) but it was also a GObject type provided by the settings
      plugin. Hence, it was not possible to migrate a profile from one plugin to
      another.
      However that would be useful when one profile does not support a
      connection type (like ifcfg-rh not supporting VPN). Currently such
      migration is not implemented except for migrating them to/from keyfile's
      run directory. The problem is that migrating profiles in general is
      complicated but in some cases it is important to do.
      
      For example checkpoint rollback should recreate the profile in the right
      settings plugin, not just add it to persistent storage. This is not yet
      properly implemented.
      
      --
      
      Previously, both keyfile and ifcfg-rh plugin implemented in-memory (unsaved)
      profiles, while ifupdown plugin cannot handle them. That meant duplication of code
      and a ifupdown profile could not be modified or made unsaved.
      This is now unified and only keyfile plugin handles in-memory profiles (bgo #744711).
      Also, NMSettings is aware of such profiles and treats them specially.
      In particular, NMSettings drives the migration between persistent and non-persistent
      storage.
      
      Note that a settings plugins may create truly generated, in-memory profiles.
      The settings plugin is free to generate and persist the profiles in any way it
      wishes. But the concept of "unsaved" profiles is now something explicitly handled
      by keyfile plugin. Also, these "unsaved" keyfile profiles are persisted to file system
      too, to the /run directory. This is great for two reasons: first of all, all
      profiles from keyfile storage in fact have a backing file -- even the
      unsaved ones. It also means you can create "unsaved" profiles in /run
      and load them with `nmcli connection load`, meaning there is a file
      based API for creating unsaved profiles.
      The other advantage is that these profiles now survive restarting
      NetworkManager. It's paramount that restarting the daemon is as
      non-disruptive as possible. Persisting unsaved files to /run improves
      here significantly.
      
      --
      
      In the past, NMSettingsConnection also implemented NMConnection interface.
      That was already changed a while ago and instead users call now
      nm_settings_connection_get_connection() to delegate to a
      NMSimpleConnection. What however still happened was that the NMConnection
      instance gets never swapped but instead the instance was modified with
      nm_connection_replace_settings_from_connection(), clear-secrets, etc.
      Change that and treat the NMConnection instance immutable. Instead of modifying
      it, reference/clone a new instance. This changes that previously when somebody
      wanted to keep a reference to an NMConnection, then the profile would be cloned.
      Now, it is supposed to be safe to reference the instance directly and everybody
      must ensure not to modify the instance. nmtst_connection_assert_unchanging()
      should help with that.
      The point is that the settings plugins may keep references to the
      NMConnection instance, and so does the NMSettingsConnection. We want
      to avoid cloning the instances as long as they are the same.
      Likewise, the device's applied connection can now also be referenced
      instead of cloning it. This is not yet done, and possibly there are
      further improvements possible.
      
      --
      
      Also implement multiple keyfile directores /usr/lib, /etc, /run (rh #1674545,
      bgo #772414).
      
      It was always the case that multiple files could provide the same UUID
      (both in case of keyfile and ifcfg-rh). For keyfile plugin, if a profile in
      read-only storage in /usr/lib gets modified, then it gets actually stored in
      /etc (or /run, if the profile is unsaved).
      
      --
      
      While at it, make /etc/network/interfaces profiles for ifupdown plugin reloadable.
      
      --
      
      https://bugzilla.gnome.org/show_bug.cgi?id=772414
      https://bugzilla.gnome.org/show_bug.cgi?id=744711
      https://bugzilla.redhat.com/show_bug.cgi?id=1674545
      d35d3c46
  2. 11 Jun, 2019 1 commit
    • Thomas Haller's avatar
      all: drop emacs file variables from source files · c0e075c9
      Thomas Haller authored
      We no longer add these. If you use Emacs, configure it yourself.
      
      Also, due to our "smart-tab" usage the editor anyway does a subpar
      job handling our tabs. However, on the upside every user can choose
      whatever tab-width he/she prefers. If "smart-tabs" are used properly
      (like we do), every tab-width will work.
      
      No manual changes, just ran commands:
      
          F=($(git grep -l -e '-\*-'))
          sed '1 { /\/\* *-\*-  *[mM]ode.*\*\/$/d }'     -i "${F[@]}"
          sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
      
      Check remaining lines with:
      
          git grep -e '-\*-'
      
      The ultimate purpose of this is to cleanup our files and eventually use
      SPDX license identifiers. For that, first get rid of the boilerplate lines.
      c0e075c9
  3. 07 May, 2019 1 commit
  4. 18 Apr, 2019 2 commits
    • Thomas Haller's avatar
      shared: move most of "shared/nm-utils" to "shared/nm-glib-aux" · d984b2ce
      Thomas Haller authored
      From the files under "shared/nm-utils" we build an internal library
      that provides glib-based helper utilities.
      
      Move the files of that basic library to a new subdirectory
      "shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
      to "libnm-glib-aux.la".
      
      Reasons:
      
       - the name "utils" is overused in our code-base. Everything's an
         "utils". Give this thing a more distinct name.
      
       - there were additional files under "shared/nm-utils", which are not
         part of this internal library "libnm-utils-base.la". All the files
         that are part of this library should be together in the same
         directory, but files that are not, should not be there.
      
       - the new name should better convey what this library is and what is isn't:
         it's a set of utilities and helper functions that extend glib with
         funcitonality that we commonly need.
      
      There are still some files left under "shared/nm-utils". They have less
      a unifying propose to be in their own directory, so I leave them there
      for now. But at least they are separate from "shared/nm-glib-aux",
      which has a very clear purpose.
      
      (cherry picked from commit 80db06f7)
      d984b2ce
    • Thomas Haller's avatar
      shared: move most of "shared/nm-utils" to "shared/nm-glib-aux" · 80db06f7
      Thomas Haller authored
      From the files under "shared/nm-utils" we build an internal library
      that provides glib-based helper utilities.
      
      Move the files of that basic library to a new subdirectory
      "shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
      to "libnm-glib-aux.la".
      
      Reasons:
      
       - the name "utils" is overused in our code-base. Everything's an
         "utils". Give this thing a more distinct name.
      
       - there were additional files under "shared/nm-utils", which are not
         part of this internal library "libnm-utils-base.la". All the files
         that are part of this library should be together in the same
         directory, but files that are not, should not be there.
      
       - the new name should better convey what this library is and what is isn't:
         it's a set of utilities and helper functions that extend glib with
         funcitonality that we commonly need.
      
      There are still some files left under "shared/nm-utils". They have less
      a unifying propose to be in their own directory, so I leave them there
      for now. But at least they are separate from "shared/nm-glib-aux",
      which has a very clear purpose.
      80db06f7
  5. 12 Feb, 2019 2 commits
  6. 29 Nov, 2018 1 commit
    • Lubomir Rintel's avatar
      all: say Wi-Fi instead of "wifi" or "WiFi" · b385ad01
      Lubomir Rintel authored
      Correct the spelling across the *entire* tree, including translations,
      comments, etc. It's easier that way.
      
      Even the places where it's not exposed to the user, such as tests, so
      that we learn how is it spelled correctly.
      b385ad01
  7. 23 Oct, 2018 3 commits
    • Andrew Zaborowski's avatar
      wifi/iwd: handle forgetting connection profiles · bea6c403
      Andrew Zaborowski authored
      Watch for connection-removed events and delete corresponding IWD network
      configs if found.  This mainly changes anything for 802.1X networks
      where the deleted NM connections might annoyingly re-appear after a
      restart.  For PSK networks though it'll make IWD forget the password
      which, until now, would be remembered by IWD even if it was removed or
      changed in the NM profile, which is a bug.
      
      This is still fragile because we don't handle "connection-updated"
      events so the data->mirror_connection pointer for a known network record
      may after some time point to an NMSettingsConnection with a different
      SSID or security type and there are corner cases where the IWD-side
      profile will not be forgotten.  At least I'm trying to make sure we
      don't crash and don't wrongly remove any IWD profile which could also be
      annoying for complicated EAP configs.
      bea6c403
    • Andrew Zaborowski's avatar
      wifi/iwd: print warning if known network exists in interface-added · b98f269b
      Andrew Zaborowski authored
      Something is possibly wrong with the DBus signal handling if a newly
      added KnownNetwork interface already has an entry in
      priv->known_networks, but since we handle this case add a warning and
      update the GDBusProxy pointer for that existing entry.
      b98f269b
    • Andrew Zaborowski's avatar
      wifi/iwd: return existing connection from mirror_8021x_connection · ba52c4ea
      Andrew Zaborowski authored
      interface_added expects mirror_8021x_connection() to return the pointer to
      the existing connection if one exists, and NULL on error, rather than
      NULL if a conneciton exists.  While touching that, add logic to return
      specifically a connection with EAP method set to 'external' if one
      exists even though this should not affect any other logic we have
      currently.
      ba52c4ea
  8. 19 Sep, 2018 2 commits
    • Andrew Zaborowski's avatar
      wifi/iwd: don't save secrets in mirror NM connections · e3aba12d
      Andrew Zaborowski authored
      When creating the mirror 802.1x connections for IWD 802.1x profiles
      set the NM_SETTING_SECRET_FLAG_NOT_SAVED flag on the secrets that
      may at some point be requested from our agent.  The saved secrets could
      not be used anyway because of our use of
      NM_SECRET_AGENT_GET_SECRETS_FLAG_REQUEST_NEW in
      nm_device_iwd_agent_query.  But also try to respect whatever secret
      caching policy has been configured in the IWD profile for those secrets,
      IWD would be responsible for storing them if it was allowed in the
      profile.
      e3aba12d
    • Andrew Zaborowski's avatar
      wifi/iwd: access Network objects through ObjectManager · fd1cfa6d
      Andrew Zaborowski authored
      In two places stop using g_dbus_proxy_new_* to create whole new
      interface proxy objects for net.connman.iwd.Network as this will
      normally have a huge overhead compared to asking the ObjectManager
      client that we already have in NMIwdManager for those proxies.
      dbus-monitor shows that for each network path returned by
      GetOrderedNetworks () -- and we call it every 10 or 20 seconds and may
      get many dozens of networks back -- gdbus would do the following each
      time:
      org.freedesktop.DBus.AddMatch("member=PropertiesChanged")
      org.freedesktop.DBus.StartServiceByName("net.connman.iwd")
      org.freedesktop.DBus.GetNameOwner("net.connman.iwd")
      org.freedesktop.DBus.Properties.GetAll("net.connman.iwd.Network")
      org.freedesktop.DBus.RemoveMatch("member=PropertiesChanged")
      fd1cfa6d
  9. 07 Sep, 2018 1 commit
    • Andrew Zaborowski's avatar
      wifi/iwd: fix leaking agent DBus objects · 910dc39c
      Andrew Zaborowski authored
      Make sure we free our IWD agent objects whenever we're freeing the
      IWD Object Manager.  We're registering those objects on the same DBus
      connection as the Object Manager so that they're visible to IWD, and
      our only reference to that connection is through priv->object_manager
      so even though the connection isn't changing when we free the object
      manager and create a new one, we still need to free the agent object.
      We could maybe keep a reference to the connection, but I'm not sure
      there's any warranty that it doesn't get closed.  We could also use
      nm_dbus_manager_get_connection (nm_dbus_manager_get ()) and only
      register and free the agent once, since it happens to be the same
      connection but it'd perhaps be a hack to rely on this.
      910dc39c
  10. 05 Sep, 2018 10 commits
    • Thomas Haller's avatar
      wifi/iwd: fix tracking of IWD-side known networks · 09988689
      Thomas Haller authored
      - since commit d17d2688, a
        NMSettingsConnection no longer "is-a" NMConnection. Instead,
        we must call nm_settings_connection_get_connection() to obtain
        the NMConnection instance. Adjust this in mirror_8021x_connection()
      
      - don't leak "ssid" in mirror_8021x_connection()
      
      - move deletion of the mirror-connection to known_network_data_free().
        Previously, we must have made sure that every g_hash_table_remove()
        and g_hash_table_insert()(!!) first deletes the mirror connection.
        Likewise, in got_object_manager() when we call g_hash_table_remove_all(),
        delete created mirror connections.
      
      - rework interface_added() to make it robust against calling
        interface_added() more than once without removing the interface
        in between. Essentially, this just means that we first look into
        "priv->known_networks" to see whether the @id is already tracked.
        And if so, delete an existing mirror-connection as necessary.
      09988689
    • Thomas Haller's avatar
      wifi/iwd: various minor cleanups in nm-iwd-manager.c · 1181f88e
      Thomas Haller authored
      - prefer "gsize" instead of "size_t".
      1181f88e
    • Thomas Haller's avatar
      wifi/iwd: use NMHashState (siphash24) for hashing · ccf36ff4
      Thomas Haller authored
      We shall use nm_hash_*() functions everywhere where
      we need a hash for a dictionary.
      ccf36ff4
    • Thomas Haller's avatar
      wifi/iwd: in manager's interface_added() ensure known-network ID is not wrongly destroyed · be875fe3
      Thomas Haller authored
      Calling g_hash_table_insert() with a key which is already hashed
      will destroy the *new* key. Since @id is used below, that would
      be use after free.
      
      Fixes: d635caf9
      be875fe3
    • Andrew Zaborowski's avatar
      wifi/iwd: Create connections for IWD-side known networks · 2c816186
      Andrew Zaborowski authored
      IWD's mechanism for connecting to EAP networks requires a network config
      file to be present in IWD's storage.  NM and its clients however won't
      allow a connection to be attempted until a valid NMConnection is created
      on the NM side for the network.  To avoid duplicating the settings from
      the IWD-side profiles in NM, automatically create NMSettingConnections
      for EAP networks preconfigured on the IWD side, unless a matching
      connection already exists.  These connections will use the "external"
      EAP method to mean their EAP settings can't be modified through NM, also
      they won't be valid for devices configured to use the wpa_supplicant
      backend unfortunately.
      
      Those nm-generated connections can be modified by NM users (makes sense
      for settings not related to the wifi authentication) in which case they
      get saved as normal profiles and will not be recreated as nm-generated
      connections on the next run.
      
      I want to additionally handle deleting connections from NM clients so
      that they're also forgotten by IWD, in a later patch.
      2c816186
    • Andrew Zaborowski's avatar
      wifi/iwd: Track known networks using interface-added/-removed signals · 142d83b0
      Andrew Zaborowski authored
      The known networks hash table is indexed by the (ssid, security) tuple
      for fast lookups both on DBus signals related to an IWD known network
      and local NMConnection signals such as on removal.
      142d83b0
    • Andrew Zaborowski's avatar
    • Andrew Zaborowski's avatar
      wifi/iwd: Drop nm_iwd_manager_network_connected · 2f941c07
      Andrew Zaborowski authored
      There's no need anymore for NMIwdManager to know when a network has been
      connected to, InterfaceAdded signals are now emitted when a network is
      saved as a Known Network.
      2f941c07
    • Andrew Zaborowski's avatar
      wifi/iwd: Drop usage of the KnownNetworks IWD API · eec61a8e
      Andrew Zaborowski authored
      Before 0.5 IWD has changed the known networks API to expose separate
      objects for each known network and dropped the KnownNetworks
      manager-like interface so stop using that interface.  Following
      patches will add tracking of the known networks through
      ObjectManager.
      eec61a8e
    • Andrew Zaborowski's avatar
      wifi/iwd: Check g_dbus_proxy_get_cached_property return values · f2be625a
      Andrew Zaborowski authored
      Instead of passing the return values to g_variant_get_string or
      g_variant_boolean and then checking the return value of that call,
      add wrappers that first check's whether the variant is non-NULL
      and of the right type.
      g_variant_get_string doesn't allow a NULL parameter and will also never
      return NULL according to the docs.
      
      For the State property we assume a state "unknown" and emit a warning
      if the property can't be read, "unknown" is also a string in IWD itself
      which would be returned if something went really wrong.  In any case
      this shouldn't happen.
      
      [thaller@redhat.com: fix missing initialization of nm_auto() variable
        interfaces.]
      f2be625a
  11. 13 Jul, 2018 1 commit
  12. 11 Jul, 2018 1 commit
    • Thomas Haller's avatar
      all: don't use gchar/gshort/gint/glong but C types · e1c7a2b5
      Thomas Haller authored
      We commonly don't use the glib typedefs for char/short/int/long,
      but their C types directly.
      
          $ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
          587
          $ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
          21114
      
      One could argue that using the glib typedefs is preferable in
      public API (of our glib based libnm library) or where it clearly
      is related to glib, like during
      
        g_object_set (obj, PROPERTY, (gint) value, NULL);
      
      However, that argument does not seem strong, because in practice we don't
      follow that argument today, and seldomly use the glib typedefs.
      Also, the style guide for this would be hard to formalize, because
      "using them where clearly related to a glib" is a very loose suggestion.
      
      Also note that glib typedefs will always just be typedefs of the
      underlying C types. There is no danger of glib changing the meaning
      of these typedefs (because that would be a major API break of glib).
      
      A simple style guide is instead: don't use these typedefs.
      
      No manual actions, I only ran the bash script:
      
        FILES=($(git ls-files '*.[hc]'))
        sed -i \
            -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
            -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>  /\1   /g' \
            -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
            "${FILES[@]}"
      e1c7a2b5
  13. 22 Jun, 2018 8 commits
  14. 30 Apr, 2018 1 commit
  15. 04 Apr, 2018 2 commits
  16. 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
         nm_manager_get_device_by_path().
       - 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.
      4a705e1a
  17. 04 Mar, 2018 2 commits
    • 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: 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