1. 17 Sep, 2018 1 commit
  2. 15 Sep, 2018 1 commit
  3. 14 Sep, 2018 2 commits
    • Thomas Haller's avatar
      libnm: drop API nm_connection_get_setting_{6lowpan,sriov,wpan}() · fe866fbe
      Thomas Haller authored
      Note that NMSettingEthtool and NMSettingMatch don't have such
      functions either.
      
      We have API
      
        nm_connection_get_setting (NMConnection *, GType)
        nm_connection_get_setting_by_name (NMConnection *, const char *)
      
      which can be used generically, meaning: the requested setting type
      is an argument to the function. That is generally more useful and
      flexible.
      
      Don't add API which duplicates existing functionality and is (arguably)
      inferiour. Drop it now. This is an ABI/API break for the current development
      cycle where the 1.14.0 API is still unstable. Indeed it's already after
      1.14-rc1, which is ugly. But it's also unlikely that somebody already uses
      this API/ABI and is badly impacted by this change.
      
      Note that nm_connection_get_setting() and nm_connection_get_setting_by_name()
      are slightly inconvenient in C still, because they usually require a cast.
      We should fix that by changing the return type to "void *". Such
      a change may be possibly any time without breaking API/ABI (almost, it'd
      be an API change when taking a function pointer without casting).
      
      (cherry picked from commit a10156f5)
      fe866fbe
    • Thomas Haller's avatar
      libnm: drop API nm_connection_get_setting_{6lowpan,sriov,wpan}() · a10156f5
      Thomas Haller authored
      Note that NMSettingEthtool and NMSettingMatch don't have such
      functions either.
      
      We have API
      
        nm_connection_get_setting (NMConnection *, GType)
        nm_connection_get_setting_by_name (NMConnection *, const char *)
      
      which can be used generically, meaning: the requested setting type
      is an argument to the function. That is generally more useful and
      flexible.
      
      Don't add API which duplicates existing functionality and is (arguably)
      inferiour. Drop it now. This is an ABI/API break for the current development
      cycle where the 1.14.0 API is still unstable. Indeed it's already after
      1.14-rc1, which is ugly. But it's also unlikely that somebody already uses
      this API/ABI and is badly impacted by this change.
      
      Note that nm_connection_get_setting() and nm_connection_get_setting_by_name()
      are slightly inconvenient in C still, because they usually require a cast.
      We should fix that by changing the return type to "void *". Such
      a change may be possibly any time without breaking API/ABI (almost, it'd
      be an API change when taking a function pointer without casting).
      a10156f5
  4. 13 Sep, 2018 2 commits
  5. 06 Sep, 2018 3 commits
    • Beniamino Galvani's avatar
      44d77a74
    • Thomas Haller's avatar
      settings: make NMSettingsPlugin a regular GObject instance and not an interface · 657b0714
      Thomas Haller authored
      NMSettingsPlugin was a glib interface, not a regular GObject
      instance. Accordingly, settings plugins would implement this interface
      instead of subclassing a parent type.
      
      Refactor the code, and make NMSettingsPlugin a GObject type. Plugins
      are now required to subclass this type.
      
      Glib interfaces are more cumbersome than helpful. At least, unless
      there is a good reason for using them.
      
      Our settings plugins are all internal API and are entirely under
      our control. It also means, this change is fine, as there are no
      implementations outside of this source tree.
      
      Using interfaces do would allow more flexibility in implementing the
      settings plugin.
      For example, the plugin would be able to derive from any other GObject
      type, like NMKimchiRefrigerator. But why would we even? Let's not add monster
      classes that implement house appliances beside NMSettingsPluginInterface.
      The settings plugin should have one purpose only: being a settings plugin.
      Hence, requiring it to subclass NMSettingsPlugin is more than resonable. We
      don't need interfaces for this.
      
      Now that NMSettingsPlugin is a regular object instance, it may also have
      state, and potentially could provide common functionality for the plugin
      implementation -- if that turns out to be useful. Arguably, an interface can
      have state too, for example by attaching the state somewhere else (like
      NMConnection does). But let's just say no.
      
      On a minor note, this also avoids some tiny overhead that comes with
      glib interfaces.
      657b0714
    • Thomas Haller's avatar
  6. 04 Sep, 2018 6 commits
    • Thomas Haller's avatar
      ifcfg-rh: don't use 802-1x certifcate setter functions · e3ac45c0
      Thomas Haller authored
      The certificate setter function like nm_setting_802_1x_set_ca_cert()
      actually load the file from disk, and validate whether it is a valid
      certificate. That is very wrong to do.
      
      For one, the certificates are external files, which are not embedded
      into the NMConnection. That means, strongly validating the files while
      loading the ifcfg files, is wrong because:
       - if validation fails, loading the file fails in its entirety with
         a warning in the log. That is not helpful to the user, who now
         can no longer use nmcli to fix the path of the certificate (because
         the profile failed to load in the first place).
       - even if the certificate is valid at load-time, there is no guarantee
         that it is valid later on, when we actually try to use the file. What
         good does such a validation do? nm_setting_802_1x_set_ca_cert() might
         make sense during nmcli_connection_modify(). At the moment when we
         create or update the profile, we do want to validate the input and
         be helpful to the user. Validating the file later on, when reloading
         the profile from disk seems undesirable.
       - note how keyfile also does not perform such validations (for good
         reasons, I presume).
      
      Also, there is so much wrong with how ifcfg reader handles EAP files.
      There is a lot of duplication, and trying to be too smart. I find it
      wrong how the "eap_readers" are nested. E.g. both eap_peap_reader() and
      "tls" method call to eap_tls_reader(), making it look like that
      NMSetting8021x can handle multiple EAP profiles separately. But it cannot. The
      802-1x profile is a flat set of properties like ca-cert and others. All
      EAP methods share these properties, so having this complex parsing
      is not only complicated, but also wrong. The reader should simply parse
      the shell variables, and let NMSetting8021x::verify() handle validation
      of the settings. Anyway, the patch does not address that.
      
      Also, the setting of the likes of NM_SETTING_802_1X_CLIENT_CERT_PASSWORD was
      awkwardly only done when
           privkey_format != NM_SETTING_802_1X_CK_FORMAT_PKCS12
        && scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11
      It is too smart. Just read it from file, if it contains invalid data, let
      verify() reject it. That is only partly addressed.
      
      Also note, how writer never actually writes the likes of
      IEEE_8021X_CLIENT_CERT_PASSWORD. That is another bug and not fixed
      either.
      e3ac45c0
    • Thomas Haller's avatar
      ifcfg-rh: rework parsing secrets · 6b763af1
      Thomas Haller authored
      - rename secret related functions to have a "_secret" prefix.
        Also, rename read_8021x_password() because it's not only useful
        for 802-1x.
      
      - In particular, this patch adds _secret_read_ifcfg() helper (formerly
        read_8021x_password()), which is smart enough to obtain secrets from
        the keys ifcfg file. We have other places where we don't get this
        right.
      
      - on a minor note, the patch also makes an effort to clear passwords
        and certifcate data from memory. Yes, there are countless places
        where we don't do that, but in this case, it's done and is as simple
        as replacing gs_free with nm_auto_free_secret, etc.
      6b763af1
    • Thomas Haller's avatar
      ifcfg-rh/trivial: rename variable for ifcfg keys file · 4b6aa207
      Thomas Haller authored
      The term "keys" is used ambigiously. Rename occurances which reference
      the "keys" ifcfg-rh file.
      
      While at it, rename the file "parsed" to "main_ifcfg". It follows the
      same pattern as the "keys_ifcfg" name.
      4b6aa207
    • Thomas Haller's avatar
      build: enable building both crypto backends for tests · e01f7f2c
      Thomas Haller authored
      If the library is available, let's at least compile both
      crypto backends.
      
      That is helpful when developing on crypto backends, so that
      one does not have to configure the build twice.
      
      With autotools, the build is only run during `make check`.
      Not for meson, but that is generally the case with our meson
      setup, that it also builds tests during the regular build step.
      e01f7f2c
    • Thomas Haller's avatar
      shared: move file-get-contents and file-set-contents helper to shared/ · ff163d9d
      Thomas Haller authored
      These functions are not specific to "src/". Also, they will be needed
      by outside of "src/" soon.
      ff163d9d
    • Thomas Haller's avatar
      core: extend nm_utils_*_get_contents() to zero temporary memory · 6b813b90
      Thomas Haller authored
      When reading a file, we may allocate intermediate buffers (realloc()).
      Also, reading might fail halfway through the process.
      
      Add a new flag that makes sure that this memory is cleared. The
      point is when reading secrets, that we don't accidentally leave
      private sensitive material in memory.
      6b813b90
  7. 30 Aug, 2018 1 commit
  8. 28 Aug, 2018 1 commit
    • Thomas Haller's avatar
      settings: use delegation instead of inheritance for NMSettingsConnection and NMConnection · 38273a88
      Thomas Haller authored
      NMConnection is an interface, which is implemented by the types
      NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
      NMRemoteConnection (libnm).
      
      NMSettingsConnection does a lot of things already:
      
        1) it "is-a" NMDBusObject and exports the API of a connection profile
           on D-Bus
        2) it interacts with NMSettings and contains functionality
           for tracking the profiles.
        3) it is the base-class of types like NMSKeyfileConnection and
           NMIfcfgConnection. These handle how the profile is persisted
           on disk.
        4) it implements NMConnection interface, to itself track the
           settings of the profile.
      
      3) and 4) would be better implemented via delegation than inheritance.
      
      Address 4) and don't let NMSettingsConnection implemente the NMConnection
      interface. Instead, a settings-connection references now a NMSimpleConnection
      instance, to which it delegates for keeping the actual profiles.
      
      Advantages:
      
        - by delegating, there is a clearer separation of what
          NMSettingsConnection does. For example, in C we often required
          casts from NMSettingsConnection to NMConnection. NMConnection
          is a very trivial object with very little logic. When we have
          a NMConnection instance at hand, it's good to know that it is
          *only* that simple instead of also being an entire
          NMSettingsConnection instance.
      
          The main purpose of this patch is to simplify the code by separating
          the NMConnection from the NMSettingsConnection. We should generally
          be aware whether we handle a NMSettingsConnection or a trivial
          NMConnection instance. Now, because NMSettingsConnection no longer
          "is-a" NMConnection, this distinction is apparent.
      
        - NMConnection is implemented as an interface and we create
          NMSimpleConnection instances whenever we need a real instance.
          In GLib, interfaces have a performance overhead, that we needlessly
          pay all the time. With this change, we no longer require
          NMConnection to be an interface. Thus, in the future we could compile
          a version of libnm-core for the daemon, where NMConnection is not an
          interface but a GObject implementation akin to NMSimpleConnection.
      
        - In the previous implementation, we cannot treat NMConnection immutable
          and copy-on-write.
          For example, when NMDevice needs a snapshot of the activated
          profile as applied-connection, all it can do is clone the entire
          NMSettingsConnection as a NMSimpleConnection.
          Likewise, when we get a NMConnection instance and want to keep
          a reference to it, we cannot do that, because we never know
          who also references and modifies the instance.
          By separating NMSettingsConnection we could in the future have
          NMConnection immutable and copy-on-write, to avoid all unnecessary
          clones.
      38273a88
  9. 22 Aug, 2018 1 commit
    • Thomas Haller's avatar
      wifi: don't use GBytesArray for NMWifiAP's ssid · 5cd4e6f3
      Thomas Haller authored
      GBytes makes more sense, because it's immutable.
      
      Also, since at other places we use GBytes, having
      different types is combersome and requires needless
      conversions.
      
      Also:
      
      - avoid nm_utils_escape_ssid() instead of _nm_utils_ssid_to_string().
        We use nm_utils_escape_ssid() when we want to log the SSID. However, it
        does not escape newlines, which is bad.
      
      - also no longer use nm_utils_same_ssid(). Since it no longer
        treated trailing NUL special, it is not different from
        g_bytes_equal().
      
      - also, don't use nm_utils_ssid_to_utf8() for logging anymore.
        For logging, _nm_utils_ssid_escape_utf8safe() is better because
        it is loss-less escaping which can be unambigously reverted.
      5cd4e6f3
  10. 11 Aug, 2018 2 commits
  11. 10 Aug, 2018 8 commits
    • Thomas Haller's avatar
      all/ethtool: add support for all currently supported kernel features · da109a29
      Thomas Haller authored
      As of upstream kernel v4.18-rc8.
      
      Note that we name the features like they are called in ethtool's
      ioctl API ETH_SS_FEATURES.
      
      Except, for features like "tx-gro", which ethtool utility aliases
      as "gro". So, for those features where ethtool has a built-in,
      alternative name, we prefer the alias.
      
      And again, note that a few aliases of ethtool utility ("sg", "tso", "tx")
      actually affect more than one underlying kernel feature.
      
      Note that 3 kernel features which are announced via ETH_SS_FEATURES are
      explicitly exluded because kernel marks them as "never_changed":
      
          #define NETIF_F_NEVER_CHANGE (NETIF_F_VLAN_CHALLENGED | \
                                        NETIF_F_LLTX | NETIF_F_NETNS_LOCAL)
      da109a29
    • Thomas Haller's avatar
      platform/ethtool: add code to get/set offload features via ethtool · c085b6e3
      Thomas Haller authored
      Also, add two more features "tx-tcp-segmentation" and
      "tx-tcp6-segmentation". There are two reasons for that:
      
       - systemd-networkd supports setting these two features,
         so lets support them too (apparently they are important
         enough for networkd).
      
       - these two features are already implicitly covered by "tso".
         Like for the "ethtool" program, "tso" is an alias for several
         actual features. By adding two features that are already
         also covered by an alias (which sets multiple kernel names
         at once), we showcase how aliases for the same feature can
         coexist. In particular, note how setting
         "tso on tx-tcp6-segmentation off" will behave as one would
         expect: all 4 tso features covered by the alias are enabled,
         except that particular one.
      c085b6e3
    • Thomas Haller's avatar
      libnm, cli, ifcfg-rh: add NMSettingEthtool setting · df30651b
      Thomas Haller authored
      Note that in NetworkManager API (D-Bus, libnm, and nmcli),
      the features are called "feature-xyz". The "feature-" prefix
      is used, because NMSettingEthtool possibly will gain support
      for options that are not only -K|--offload|--features, for
      example -C|--coalesce.
      
      The "xzy" suffix is either how ethtool utility calls the feature
      ("tso", "rx"). Or, if ethtool utility specifies no alias for that
      feature, it's the name from kernel's ETH_SS_FEATURES ("tx-tcp6-segmentation").
      If possible, we prefer ethtool utility's naming.
      
      Also note, how the features "feature-sg", "feature-tso", and
      "feature-tx" actually refer to multiple underlying kernel features
      at once. This too follows what ethtool utility does.
      
      The functionality is not yet implemented server-side.
      df30651b
    • Thomas Haller's avatar
      ifcfg-rh: refactor parsing in parse_ethtool_option() to not call helper functions · bcbea6fe
      Thomas Haller authored
      Parsing can be complicated enough. It's simpler to just work
      top-to-bottom, without calling various helper functions. This was,
      you can see all the code in one place, without need to jump to
      the helper function to see what it is doing.
      
      In general, a static function that is only called once, does sometimes
      not simplify but obfuscate the code.
      bcbea6fe
    • Thomas Haller's avatar
      ifcfg-rh: always reset ETHTOOL_WAKE_ON_LAN value · 64e0e241
      Thomas Haller authored
      We must always set all variables, because othewise a previously set
      value might be merged into the new setting.
      64e0e241
    • Thomas Haller's avatar
      ifcfg-rh: split setting ETHTOOL_OPTS from write_wired_setting() · cd442112
      Thomas Haller authored
      Will be used later, because we will not only have ethtool options
      in conjunction with wired settings.
      cd442112
    • Thomas Haller's avatar
      ifcfg-rh: cleanup write_wired_setting() · 1bcf1047
      Thomas Haller authored
      Drop some local variables, or move them inside a nested scope,
      closer to where they are used.
      1bcf1047
    • Thomas Haller's avatar
      ifcfg-rh/tests: regenerate .cexpected files with NM_TEST_REGENERATE=1 · f69fb04c
      Thomas Haller authored
      The tests already honored the environment variable $NMTST_IFCFG_RH_UPDATE_EXPECTED
      to indicate that the .cexpected files should be written by the tests.
      
      However, in the meantime, we instead use NM_TEST_REGENERATE=1 at various
      places for this purpose. Honor that flag as well.
      f69fb04c
  12. 08 Aug, 2018 1 commit
    • Thomas Haller's avatar
      all: add connection.multi-connect property for wildcard profiles · 55ae6923
      Thomas Haller authored
      Add a new option that allows to activate a profile multiple times
      (at the same time). Previoulsy, all profiles were implicitly
      NM_SETTING_CONNECTION_MULTI_CONNECT_SINGLE, meaning, that activating
      a profile that is already active will deactivate it first.
      
      This will make more sense, as we also add more match-options how
      profiles can be restricted to particular devices. We already have
      connection.type, connection.interface-name, and (ethernet|wifi).mac-address
      to restrict a profile to particular devices. For example, it is however
      not possible to specify a wildcard like "eth*" to match a profile to
      a set of devices by interface-name. That is another missing feature,
      and once we extend the matching capabilities, it makes more sense to
      activate a profile multiple times.
      
      See also https://bugzilla.redhat.com/show_bug.cgi?id=997998, which
      previously changed that a connection is restricted to a single activation
      at a time. This work relaxes that again.
      
      This only adds the new property, it is not used nor implemented yet.
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1555012
      55ae6923
  13. 17 Jul, 2018 1 commit
    • Thomas Haller's avatar
      build: create "config-extra.h" header instead of passing directory variables via CFLAGS · a75ab799
      Thomas Haller authored
      1) the command line gets shorter. I frequently run `make V=1` to see
         the command line arguments for the compiler, and there is a lot
         of noise.
      
      2) define each of these variables at one place. This makes it easy
         to verify that for all compilation units, a particular
         define has the same value. Previously that was not obvious or
         even not the case (see commit e5d1a713
         and commit d63cf1ef).
         The point is to avoid redundancy.
      
      3) not all compilation units need all defines. In fact, most modules
         would only need a few of these defines. We aimed to pass the necessary
         minium of defines to each compilation unit, but that was non-obvious
         to get right and often we set a define that wasn't used. See for example
         "src_settings_plugins_ibft_cppflags" which needlessly had "-DSYSCONFDIR".
         This question is now entirely avoided by just defini...
      a75ab799
  14. 11 Jul, 2018 3 commits
    • Beniamino Galvani's avatar
      ifcfg-rh: SR-IOV support · c02d1c48
      Beniamino Galvani authored
      c02d1c48
    • Beniamino Galvani's avatar
      ifcfg-rh: add @match_key_type argument to svGetKeys() · 347e0d8b
      Beniamino Galvani authored
      Add a @match_key_type to svGetKeys() to filter the keys to be returned.
      347e0d8b
    • 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
  15. 15 Jun, 2018 1 commit
  16. 11 Jun, 2018 1 commit
    • Lubomir Rintel's avatar
      ifcfg-rh: add nm-ifup and nm-ifdown scripts · d8151304
      Lubomir Rintel authored
      They're intended to be used via update-alternatives(8) as compatibility
      shims for Red Hat systems without the legacy network control scripts.
      
      While they're not strictly parts of the settings plugin, they're best
      just installed along with it, since they're supposed to be available on
      systems that use the ifcfg files.
      d8151304
  17. 09 Jun, 2018 3 commits
  18. 31 May, 2018 2 commits