1. 20 Nov, 2018 4 commits
    • Beniamino Galvani's avatar
      cli: wait for changed signal after updating a connection · a370faeb
      Beniamino Galvani authored
      In editor_menu_main(), after saving a connection we wait that the
      Update2() D-Bus call returns and then we copy the NMRemoteConnection
      instance over to @connection.
      
      This assumes that when Update2() returns the remote connection
      instance is already updated with new settings. Indeed, on server side
      the NMSettingsConnection first emits the "Updated" signal and then
      returns to Update2(). However, the Updated signal doesn't include the
      new setting values and so libnm has to fire an asynchronous
      nmdbus_settings_connection_call_get_setting() to fetch the new
      settings, which terminates after the Update2().
      
      So, to be sure that the remote connection got updated we have also to
      listen to the connection-changed signal, which is always emitted after
      an update.
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1546805
      a370faeb
    • Beniamino Galvani's avatar
      cli: editor: reload secrets after updating connection · 096eef61
      Beniamino Galvani authored
      Connection secrets are lost after calling
      nm_connection_replace_settings_from_connection() because @con_tmp
      doesn't contain secrets; refetch them like we do when starting the
      editor.
      096eef61
    • Beniamino Galvani's avatar
      libnm-core: macsec: don't require a cak in verify() · 8d5b0161
      Beniamino Galvani authored
      CAK is a connection secret and can be NULL for various reasons
      (agent-owned, no permissions to get secrets, etc.). verify() must not
      require it.
      
      Fixes: 474a0dbf
      8d5b0161
    • Thomas Haller's avatar
      keep-alive: check GetNameOwner lazy and only when we rely on the information · a7640618
      Thomas Haller authored
      Upside:
      
        - it may avoid a D-Bus call altogether.
      
        - currently there is no API to bind/unbind an existing NMActiveConnection,
          it can only be bound initially with AddAndActivate2.
          That makes sense when the calling applicatiion only wants to keep the
          profile active for as long as it lives. It never intends to extend the
          lifetime beyond that. In such a case, it is expected that eventually
          we need to be sure whether the D-Bus client is still alive, as we
          make a decision based on that. In that case, we eventually will call
          GetNameOwner and nothing is saved.
          A future feature could add D-Bus API to bind/unbind existing active
          connections after creation. That would allow an application to
          activate profiles and -- if anything goes wrong -- to be sure
          that the profile gets deactivted. Only in the success case, it
          would unbind the lifetime in a last step and terminate. Un such
          a case, binding to a D-Bus client is more of a fail-safe mechanism
          and commonly not expected to come into action.
          This is an interesting feature, because ActivateConnection D-Bus call
          may take a long time, while perfroming interactive polkit authentication.
          That means, `nmcli connection up $PROFILE` can fail with timeout before
          the active connection path is even created, but afterwards the profile may
          still succeed to activate. That seems wrong. nmcli could avoid that,
          by binding the active connection to itself, and when nmcli exits
          with failure, it would make sure that the active connection gets
          disconnected as well.
      
      Downside:
      
        - the code is slightly more complicated(?).
      
        - if the D-Bus name is indeed gone (but the NMKeepAlive is still alive
          for other reasons), we will not unregister the D-Bus signal, because we
          never learn that it's gone. It's not a leak, but an unnecessary
          signal subscription.
      
        - during nm_keep_alive_set_dbus_client_watch() we don't notice right
          away that the D-Bus client is already gone. That is unavoidable as
          we confirm the state asynchronously.
          If the NMKeepAlive is kept alive for other reasons but only later
          depends on presence of the D-Bus client, then in the non-lazy approach
          we would have noticed already that the client is gone, and would disconnect
          right away. With the lazy approach, this takes another async D-Bus call to
          notice.
      a7640618
  2. 19 Nov, 2018 5 commits
  3. 18 Nov, 2018 4 commits
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      manager: prefer nm_streq over strcmp in impl_manager_add_and_activate_connection() · 28c386df
      Thomas Haller authored
      - use nm_streq() instead of g_strcmp0(). I think streq() is easier
        to understand.
      
      - the strings that are checked here must never be %NULL, because they come
        from string variants. Use nm_streq() instead of nm_streq0() or g_strcmp0().
      
      - don't add a "." to the GError messages. GError messages are commonly
        embedded in a larger message, and shoult not themself contain the dot.
      28c386df
    • Thomas Haller's avatar
      manager: fix checking for bind-lifetime setting in add-and-activate · bc23dc8f
      Thomas Haller authored
      Previously, @bind_lifetime was a string. While parsing the @options, we
      would set the string to the content of the parsed GVariant. Note that
      the GVariant is unrefed before we access @bind_lifetime, thus it's
      not guaranteed that it will still exist.
      
      Arguably, the string GVariant's lifetime is tied to the @options
      dictionary, so indeed it lives long enough. But that is not-obviously
      the case.
      
      Fix that by using a boolean instead. Also, rename @bind_lifetime to
      @bind_dbus_client.
      bc23dc8f
    • Thomas Haller's avatar
      manager: allow add-and-activate option "bind" with non-volatile profiles · 6f28f4b6
      Thomas Haller authored
      For one, there was a bug here: we cannot "goto error" without setting
      the @error variable.
      
      Anyway, restricting "bind" "dbus-client" only to profiles that are
      "persist" mode "volatile" seems wrong. The "bind" option as it is,
      limits the lifetime of the active-connection. This has no direct relation
      with the lifetime of the setting-connection. Indeed, if the
      settings-connection's lifetime is itself set to "volatile", then
      it will indeed go away with the active-connection. However, these
      two concepts are not strictly related.
      
      In the future, we might add an option to limite the lifetime of
      a settings-connection to a D-Bus client ("bind-setting"). Possibly
      we should thus rename "bind" to "bind-activation", to make the
      distinction clearer.
      6f28f4b6
  4. 17 Nov, 2018 11 commits
    • Thomas Haller's avatar
      policy: don't check for valid error in active_connection_keep_alive_changed() · f10f0199
      Thomas Haller authored
      Most (not all) functions that can fail and report the reason with
      an GError are required to set the error if they fail. It's a bug
      to claim to fail without returning the GError reason.
      
      Hence, our callers usually don't check whether a GError is present but
      just access it.
      
      Likewise, for better or worse, our GError codes are often not meaningful
      (unless explicitly documented). Meaning, logging the error code number
      is not helpful. Instead, error messages should be written in a manner
      that one can find the source code location where it happened.
      
      Also, return-early to reduce the indentation level of the code.
      
      Also, drop the code comment. It seems to just describe what is obviously
      visible by reading the source. It doesn't explain much beside that the
      "doesn't have a reason", but not really why.
      f10f0199
    • Thomas Haller's avatar
      keep-alive: minor cleanup of nm_keep_alive_set_dbus_client_watch() · 6b79af28
      Thomas Haller authored
      - always issue a _notify_alive(), just to be sure. At least
        in case where we clear a dbus-client watch, the alive state
        could change.
      
      - avoid the logging in cleanup_dbus_watch(), if there is nothing
        to cleanup.
      6b79af28
    • Thomas Haller's avatar
      keep-alive: cleanup tracking visible state of connection · 3baf56b4
      Thomas Haller authored
      - in nm_keep_alive_set_settings_connection_watch_visible(), abort
        early when setting the same connection again (or %NULL again).
      
      - nm_keep_alive_set_settings_connection_watch_visible() would (already
        before) correctly disconnect the signal handler from the previous
        connection.
        As we anyway do explict disconnects, avoid the overhead of a weak
        pointer with g_signal_connect_object(). Just reuse the same
        setter to disconnect the signal in dispose().
      
      - fix leaking priv->connection in nm_keep_alive_set_settings_connection_watch_visible().
      
      - use g_signal_handlers_disconnect_by_func(), to be more specific about
        which signal we want to disconnect.
      3baf56b4
    • Thomas Haller's avatar
      keep-alive: cache the alive-state and only emit the signal when it changed · be91d1cc
      Thomas Haller authored
      The alive state is composed from various parts, let's only emit
      a _notify (self, PROP_ALIVE) when it actually changes.
      
      For that, cache the alive state and let _notify_alive() determine
      whether it changed and possibly emit the signal.
      be91d1cc
    • Thomas Haller's avatar
      keep-alive: various style fixes · 58923de4
      Thomas Haller authored
      Some trivial changes:
      
      - move nm_keep_alive_new() after nm_keep_alive_init(), so that the
        functions that initialize the instance are beside each other.
      - prefer nm_streq*() over strcmp().
      - wrap some lines.
      - remove some empty lines.
      58923de4
    • Thomas Haller's avatar
      keep-alive: drop unused error argument · 7842a580
      Thomas Haller authored
      7842a580
    • Benjamin Berg's avatar
      libnm: Add support to pass options to AddAndActivateConnection · 00236ef9
      Benjamin Berg authored
      This adds the new methods nm_client_add_and_activate_connection_options_*
      and ports the existing methods to use the new AddAndActivateConnection2
      call rather than AddAndActivateConnection, allowing further parameters
      to be passed in.
      00236ef9
    • Benjamin Berg's avatar
      core: Add option to AddAndActivateConnection2 to bind the lifetime · eb883e34
      Benjamin Berg authored
      This allows binding the lifetime of the created connection to the
      existance of the requesting dbus client. This feature is useful if one
      has a service specific connection (e.g. P2P wireless) which will not be
      useful without the specific service.
      
      This is simply a mechanism to ensure proper connection cleanup if the
      requesting service has a failure.
      eb883e34
    • Benjamin Berg's avatar
      core: Add persist option to AddAndActivateConnection2 · 9cef6483
      Benjamin Berg authored
      This option allows setting the rules for how long the connection should
      be stored. Valid values are "disk" (the default), "memory" and
      "volatile". If "memory" or "volatile" is selected, the connection will
      not be saved to disk and with "volatile" it will be automatically
      removed when it is deactivated again.
      9cef6483
    • Benjamin Berg's avatar
      core: Add an AddAndActivateConnection2 routine with options parameter · 43c19d75
      Benjamin Berg authored
      This adds a new routine to be able to handle an arbitrary set of further
      options for AddAndActivateConnection. Note that no options are accepted
      for now.
      43c19d75
    • Benjamin Berg's avatar
      core: Introduce helper class to track connection keep alive · 37e8c53e
      Benjamin Berg authored
      For P2P connections it makes sense to bind the connection to the status
      of the operation that is being done. One example is that a wifi display
      (miracast) P2P connection should be shut down when streaming fails for
      some reason.
      
      This new helper class allows binding a connection to the presence of a
      DBus path meaning that it will be torn down if the process disappears.
      37e8c53e
  5. 16 Nov, 2018 1 commit
  6. 15 Nov, 2018 3 commits
  7. 14 Nov, 2018 5 commits
  8. 13 Nov, 2018 7 commits
    • Thomas Haller's avatar
      e9630e7d
    • Thomas Haller's avatar
      dhcp: add "ipv4.dhcp-client-id=duid" setting · 8861ac29
      Thomas Haller authored
      Add a new mode for the DHCPv4 client identifier.
      
      "duid" is what the internal (systemd) DHCP client already does by
      default. It is also the same as used by systemd-networkd's
      "ClientIdentifier=duid" setting. What we still lack (compared to
      networkd) are a way to overwrite IAID and the DUID.
      
      Previously, this mode was used by the internal DHCP plugin
      by default. However, it could not be explicitly configured.
      In general, our default values should also be explicitly selectable.
      Now the "duid" client identifier can also be used with the "dhclient"
      plugin.
      8861ac29
    • Thomas Haller's avatar
      dhcp: always require hwaddr in internal DHCP clint ip6_start() · dfdd4e3b
      Thomas Haller authored
      Note how client_start() in NMDhcpManager already asserts
      that we have a MAC address. It's always present, like
      for the IPv6 case.
      dfdd4e3b
    • Thomas Haller's avatar
      5ef93c33
    • Thomas Haller's avatar
      all: add "${MAC}" substituion for "connection.stable-id" · 7ffbf712
      Thomas Haller authored
      We already had "${DEVICE}" which uses the interface name.
      In times of predictable interface naming, that works well.
      It allows the user to generate IDs per device which don't
      change when the hardware is replaced.
      
      "${MAC}" is similar, except that is uses the permanent MAC
      address of the device. The substitution results in the empty
      word, if the device has no permanent MAC address (like software
      devices).
      
      The per-device substitutions "${DEVICE}" and "${MAC}" are especially
      interesting with "connection.multi-connect=multiple".
      7ffbf712
    • Thomas Haller's avatar
      dhcp: cleanup error handling in internal DHCP client's start · ab314065
      Thomas Haller authored
      - use nm_auto to return early when something goes wrong
      
      - don't modify NMDhcpClient's state until the end, when it looks
        like we are (almost) started successfully.
      
      - for IPv4, only attempt to load the lease if we actually are
        interested in the address. Also, reduce the scope of the lease
        variable, to the one place where we need it.
      ab314065
    • Thomas Haller's avatar
      dhcp: don't load IPv4 client-id from lease file · 5b9bc174
      Thomas Haller authored
      The client-id is something that we want to determine top-down.
      Meaning, if the user specifies it via ipv4.dhcp-client-id, then it
      should be used. If the user leaves it unspecified, we choose a
      default stable client-id. For the internal DHCP plugin, this is
      a node specific client-id based on
      
        - the predictable interface name
        - and /etc/machine-id
      
      It's not clear, why we should allow specifying the client-id in
      the lease file as a third source of configuration. It really pushes
      the configuration first down (when we do DHCP without lease file),
      to store an additional bit of configuration for future DHCP attempts.
      
      If the machine-id or the interface-name changes, then so does the
      default client-id. In this case, also "ipv4.dhcp-client-id=stable"
      changes. It's fair to require that the user keeps the machine-id
      stable, if the machine identity doesn't change.
      
      Also, the lease files are stored in /var/lib/NetworkManager, which
      is more volatile than /etc/machine-id. So, if we think that machine-id
      and interface-name is not stable, why would we assume that we have
      a suitable lease file?
      
      Also, if you do:
      
         nmcli connection add con-name "$PROFILE" ... ipv4.dhcp-client-id ''
         nmcli connection up $PROFILE
         nmcli connection modify "$PROFILE" ipv4.dhcp-client-id mac
         nmcli connection up $PROFILE
         nmcli connection modify "$PROFILE" ipv4.dhcp-client-id ''
         nmcli connection up $PROFILE
      
      wouldn't you expect that the original (default) client-id is used again?
      
      Also, this works badly with global connection defaults in
      NetworkManager.conf. If you configure a connection default, previously
      already this would always force the client-id and overrule the lease.
      That is reasonable, but in which case would you ever want to use
      the client-id from the lease?
      5b9bc174