1. 11 Dec, 2018 1 commit
  2. 13 Nov, 2018 1 commit
    • Thomas Haller's avatar
      all: cleanup GChecksum handling · eb9f950a
      Thomas Haller authored
      - prefer nm_auto_free_checksum over explicit free.
      - use nm_utils_checksum_get_digest*().
      - prefer defines for digest length.
      - assume g_checksum_new() cannot fail.
      eb9f950a
  3. 12 Nov, 2018 4 commits
  4. 27 Sep, 2018 1 commit
  5. 26 Sep, 2018 2 commits
  6. 24 Sep, 2018 1 commit
    • Lubomir Rintel's avatar
      dns: allow loading nm-dns-systemd-resolve alongside other DNS plugins · d4eb4cb4
      Lubomir Rintel authored
      Even when the system resolver is configured to something else that
      systemd-resolved, it still is a good idea to keep systemd-resolved up to
      date. If not anything else, it does a good job at doing per-interface
      resolving for connectivity checks.
      
      If for whatever reasons don't want NetworkManager to push the DNS data
      it discovers to systemd-resolved, the functionality can be disabled
      with:
      
        [main]
        systemd-resolved=false
      d4eb4cb4
  7. 21 Sep, 2018 4 commits
    • Thomas Haller's avatar
      dns: fix creating resolv.conf content · 511709c5
      Thomas Haller authored
      g_string_new_len() allocates the buffer with length
      bytes. Maybe it should be obvious (wasn't to me), but
      if a init argument is given, that is taken as containing
      length bytes.
      
      So,
      
          str = g_string_new_len (init, len);
      
      is more like
      
          str = g_string_new_len (NULL, len);
          g_string_append_len (str, init, len);
      
      and not (how I wrongly thought)
      
          str = g_string_new_len (NULL, len);
          g_string_append (str, init);
      
      Fixes: 95b006c2
      511709c5
    • Thomas Haller's avatar
      dns: always write "/var/run/NetworkManager/resolv.conf" · cddb9132
      Thomas Haller authored
      Previously, if "main.rc-manager" was set to "unmanaged"
      and "/etc/resolv.conf" was symlink to our internal file
      "/var/run/NetworkManager/resolv.conf", NM would not rewrite
      the file, in an attempt to honor the requirement of NetworkManager
      not changing resolv.conf.
      
      No longer special case this. I think it was wrong and inconsistent.
      If the user specifies rc-manager unmanaged, he also should manage
      /etc/resolv.conf accordingly. And if the user decided to symlink
      it to our internal file, that is fine. It should not stop NM from
      updating that file.
      
      Also, this was the only cases, where NM would not write our internal
      resolv.conf (errors aside). It was inconsitent, and also not documented
      behavior. Instead, it is documented that `man NetworkManager.conf`:
      
        Regardless of this setting, NetworkManager will always write
        resolv.conf to its runtime state directory
        /var/run/NetworkManager/resolv.conf.
      cddb9132
    • Thomas Haller's avatar
      dns: write original DNS servers to /var/run/NetworkManager/no-stub-resolv.conf · 0dc673f0
      Thomas Haller authored
      When a DNS plugin is enabled (like "main.dns=dnsmasq" or "main.dns=systemd-resolved"),
      the name servers announced to the rc-manager are coerced to be 127.0.0.1
      or 127.0.0.53.
      
      Depending on the "main.rc-manager" setting, also "/etc/resolv.conf"
      contains only this coerced name server to the local caching service.
      The same is true for "/var/run/NetworkManager/resolv.conf" file, which
      contains what we would write to "/etc/resolv.conf" (depending on
      the "main.rc-manager" configuration).
      
      Write a new file "/var/run/NetworkManager/no-stub-resolv.conf", which contains
      the original name servers, uncoerced. Like "/var/run/NetworkManager/resolv.conf",
      this file is always written.
      
      The effect is, when one enables "main.dns=systemd-resolved", then there
      is still a file "no-stub-resolv.conf" with the same content as with
      "main.dns=default".
      
      The no-stub-resolv.conf may be a possible solution, when a user wants
      NetworkManager to update systemd-resolved, but still have a regular
      /etc/resolv.conf [1]. For that, the user could configure
      
          [main]
          dns=systemd-resolved
          rc-manager=unmanaged
      
      and symlink "/etc/resolv.conf" to "/var/run/NetworkManager/no-stub-resolv.conf".
      This is not necessarily the only solution for the problem and does not preclude
      options for updating systemd-resolved in combination with other DNS plugins.
      
      [1] #20
      0dc673f0
    • Thomas Haller's avatar
  8. 06 Sep, 2018 1 commit
    • Beniamino Galvani's avatar
      core: nm-ip4-config: consider dns-related differences as relevant · 6169cd57
      Beniamino Galvani authored
      The DNS manager reacts to NM_DEVICE_IP4_CONFIG_CHANGED events, which
      are generated when there is a relevant change to an IP4 configuration.
      
      Until now, changes to the mdns,llmnr properties were not
      considered relevant (and neither minor, this is already a bug).
      
      Promote them to relevant so that the DNS manager is notified and will
      rewrite the DNS configuration when one of this properties changes.
      
      Note that the DNS priority should be considered relevant and added
      into the checksum as well, but is a problem right now because in the
      DNS manager we rely on the fact that an empty configuration (i.e. just
      created) has a zero checksum. This is needed to avoid rewriting
      resolv.conf when there is no change. The DNS priority initial value
      depends on the connection type (VPN or not), so it's a bit difficult
      to add it to checksum maintaining the assumption of checksum(empty)=0.
      This should be improved in the future.
      6169cd57
  9. 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 gui...
      e1c7a2b5
  10. 05 Jun, 2018 1 commit
    • Thomas Haller's avatar
      dns: change main.rc-manager=file behavior to always follow symlink · 644aa42f
      Thomas Haller authored
      With "main.rc-manager=file", if /etc/resolv.conf is a symlink, NetworkManager
      would follow the symlink and update the file instead.
      
      However, note that realpath() only returns a target, if the file actually
      exists. That means, if /etc/resolv.conf is a dangling symlink, NetworkManager
      would replace the symlink with a file.
      
      This was the only case in which NetworkManager would every change a symlink
      resolv.conf to a file. I think this is undesired behavior.
      
      This is a change in long established behavior. Although note that there were several
      changes regarding rc-manager settings in the past. See for example commit [1] and [2].
      
      Now, first still try using realpath() as before. Only if that fails, try
      to resolve /etc/resolv.conf as a symlink with readlink().
      
      Following the dangling symlink is likely not a problem for the user, it
      probably is even desired. The part that most likely can cause problems
      is if the destination file is not writable. That happens for example, if
      the destination's parent directories are missing. In this case, NetworkManager
      will now fail to write resolv.conf and log a warning. This has the potential of
      breaking existing setups, but it really is a mis-configuration from the user's
      side.
      
      This fixes for example the problem, if the user configures
      /etc/resolv.conf as symlink to /tmp/my-resolv.conf. At boot, the file
      would not exist, and NetworkManager would previously always replace the
      link with a plain file. Instead, it should follow the symlink and create
      the file.
      
      [1] 718fd224
      [2] 15177a34
      
      https://github.com/NetworkManager/NetworkManager/pull/127
      644aa42f
  11. 01 Jun, 2018 2 commits
  12. 26 May, 2018 1 commit
  13. 14 May, 2018 1 commit
    • Beniamino Galvani's avatar
      dns: use dns-priority to provide a preprocessed domain list to plugins · dd1e671f
      Beniamino Galvani authored
      Do some preprocessing on the DNS configuration sent to plugins:
      
       - add the '~' default routing (lookup) domain to IP configurations
         with the default route or, when there is none, to all non-VPN
         IP configurations
      
       - use the dns-priority to decide which connection to use in case
         multiple connections have the same domain
      
       - consider a negative dns-priority value as a way to 'shadow' all
         subdomains from other connections
      
       - compute reverse DNS domains
      
      and add the resulting domain list to NMDnsIPConfigData so that
      split-DNS plugins can use that directly instead of reimplementing the
      same logic themselves.
      dd1e671f
  14. 30 Apr, 2018 1 commit
  15. 04 Apr, 2018 1 commit
  16. 27 Mar, 2018 1 commit
    • Thomas Haller's avatar
      config: cleanup fields in NMGlobalDnsConfig · cd48bc74
      Thomas Haller authored
      - consistently set options, searches, domains fields to %NULL,
        if there are no values.
      
      - in nm_global_dns_config_update_checksum(), ensure that we uniquely
        hash values. E.g. a config with "searches[a], options=[b]" should
        hash differently from "searches=[ab], options=[]".
      
      - in nm_global_dns_config_to_dbus(), reuse the sorted domain list.
        We already have it, and it guarantees a consistent ordering of
        fields.
      
      - in global_dns_domain_from_dbus(), fix memleaks if D-Bus strdict
        contains duplicate entries.
      cd48bc74
  17. 20 Mar, 2018 1 commit
  18. 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.
      57ab9fd6
  19. 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
      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
  20. 10 Mar, 2018 1 commit
  21. 09 Feb, 2018 1 commit
  22. 12 Jan, 2018 4 commits
  23. 09 Jan, 2018 5 commits
    • Thomas Haller's avatar
      core: rework tracking config in dns-manager to use ifindex · b40729ca
      Thomas Haller authored
      Don't track the per-device configuration in NMDnsManager by
      the ifname, but by the ifindex. We should consistently treat
      the ifindex as the ID of a link, like kernel does.
      
      At the few places where we actually need the ifname, resolve
      it by looking into the platform cache. That is not necessarily
      the same as the ifname that is currently tracked by NMDevice,
      because netdev interfaces can be renamed, and NMDevice updates
      it's link properties delayed. However, the platform cache has
      the most recent notion of the correct interface name for an
      ifindex, so if we ever hit a race here, we do it now more
      correctly.
      
      This also temporarily drops support for mdns. Will be re-added next,
      but differently.
      b40729ca
    • Thomas Haller's avatar
      core/trivial: rename local variable in merge_one_ip_config() · fc40d91b
      Thomas Haller authored
      Next commit will unify naming of variables, do a trivial rename
      first to make the diff smaller.
      fc40d91b
    • Thomas Haller's avatar
      libnm: rename MDns flag UNKNOWN to DEFAULT · 9d92848a
      Thomas Haller authored
      "UNKNOWN" is not a good name. If you don't set the property
      in the connection explicitly, it should be "DEFAULT".
      
      Also, make "DEFAULT" -1. For one, that ensures that the enum's
      underlying integer type is signed. Otherwise, it's cumbersome
      to test "if (mdns >= DEFAULT)" because in case of unsigned types,
      the compiler will warn about the check always being true.
      Also, it allows for "NO" to be zero. These are no strong reasons,
      but I tend to think this is better.
      
      Also, don't make the property of NMSettingConnection a CONSTRUCT property.
      Initialize the default manually in the init function.
      
      Also, order the numeric values so that DEFAULT < NO < RESOLVE < YES with
      YES being largest because it enables *the most*.
      9d92848a
    • Ismo Puustinen's avatar
      dns: add mechanism for propagating mDNS setting. · 25906eda
      Ismo Puustinen authored
      Update nm-policy.c and nm-dns-manager.c so that the connection-specific
      settings get propagated to DNS manger. Currently the only such value is
      the mDNS status.
      
      Add update_mdns() function to DNS plugin interface. If a DNS plugin
      supports mDNS, it can set an interface with a given index to support
      mDNS resolving or also register the current hostname.
      
      The mDNS support is currently added only to systemd-resolved DNS plugin.
      25906eda
    • Thomas Haller's avatar
      core: reorder code in "src/dns/nm-dns-manager.c" · 3d86429c
      Thomas Haller authored
      Just moving code around, no other changes.
      
      Follow a certain prefered order of declarations
      in source files.
      3d86429c
  24. 18 Dec, 2017 2 commits
    • Thomas Haller's avatar
      dns: rework write_to_netconfig() · 41f608dd
      Thomas Haller authored
      The compiler warns when we ignore the return value from write().
      And assigning it to an unused variable, causes another warning.
      Make some use of it, at least to handle EINTR. All other errors
      are still ignored.
      
      While at it, rework the write code to first write to a buffer
      in memory.
      41f608dd
    • Thomas Haller's avatar
      core: avoid compiler warnings in write_to_netconfig() and ifnet_update_parsers_by_connection() · ad3bbda8
      Thomas Haller authored
          src/dns/nm-dns-manager.c: In function ‘write_to_netconfig’:
          src/dns/nm-dns-manager.c:387:8: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
            write (fd, str, strlen (str));
                  ^
      
          src/settings/plugins/ifnet/nms-ifnet-connection-parser.c: In function ‘ifnet_update_parsers_by_connection’:
          src/settings/plugins/ifnet/nms-ifnet-connection-parser.c:2600:26: error: variable ‘pppoe’ set but not used [-Werror=unused-but-set-variable]
            gboolean wired = FALSE, pppoe = TRUE;
                                    ^~~~~
      
      While at it, don't log line breaks in ifnet_update_parsers_by_connection().
      
      Fixes: e912b36d
      ad3bbda8