1. 09 Dec, 2018 14 commits
    • Thomas Haller's avatar
      keep-alive: drop unused nm_keep_alive_set_forced() · c9354cb4
      Thomas Haller authored
      set-forced is currently unused, so drop it.
      
      NMKeepAlive in principle determines the alive-status based on multiple
      aspects, that in combination render the instance alive or dead. These
      aspects cooperate in a particular way.
      
      By default, a keep-alive instance should be alive. If there are conditions
      enabled that further determine the alive-state, then these conditions
      cooperate in a particular way. As it was, the force-flag would just
      overrule them all.
      
      But is that useful? The nm_keep_alive_set_forced() API also means that only
      one user caller can have control over the flag. Independent callers cannot
      cooperate on setting the flag, because there is no reference-counting or
      registered handles.
      
      At least today, it's unclear whether this flag really should overrule all
      other conditions and how this flag would actually be used. Drop it for
      now.
      c9354cb4
    • Thomas Haller's avatar
      keep-alive: use NMKeepAlive API directly instead of via NMActiveConnection · f95a5263
      Thomas Haller authored
      NMKeepAlive is a proper GObject type, with a specific API that on the one
      end allows to configure watches/bindings, and on the other end exposes
      and is-alive property and the owner instance. That's great, as NMActiveConnection
      is not concerned with either end (moving complexity away from
      "nm-active-connection.c") and as we later can reuse NMKeepAlive with
      NMSettingsConnection.
      
      However, we don't need to wrap this API by NMActiveConnection. Doing so
      means, we need to expose all the watch/bind functions also as part of
      NMActiveConnection API.
      
      The only ugliness here is, that NMPolicy subscribes to property changed
      signal of the keep alive instance, which would fail horribly if
      NMActiveConnection ever decides to swap the keep alive instance (in
      which case NMPolicy would have to disconnect the signal, and possibly
      reconnect it to another NMKeepAlive instance). We avoid that by just not
      doing that and documenting it.
      f95a5263
    • Thomas Haller's avatar
      keep-alive: let NMKeepAlive instance access the owner object that it is keeping alive · 9e8c3d2e
      Thomas Haller authored
      NMKeepAlive is a full API independent of NMActiveConnection. For good
      reasons:
      
        - it moves complexity away from nm-active-connection.c
      
        - in the future, we can use NMKeepAlive also for NMSettingsConnection
      
      As such, the user should also directly interact with NMKeepAlive,
      instead of going through NMActiveConnection. For that to work, it
      must be possible to get the owner of the NMKeepAlive instance,
      which is kept alive.
      9e8c3d2e
    • Thomas Haller's avatar
      manager: use nm_device_disconnect_active_connection() in nm_manager_deactivate_connection() · 023ebd8e
      Thomas Haller authored
      We should not blindly change the device's state. Instead, call
      nm_device_disconnect_active_connection() which will figure out whether
      the device state needs to change. Note that it is very possible that
      the active-connection instance is still queued, not yet queued, or
      already disconnected. nm_device_disconnect_active_connection() does
      the right thing in all cases.
      023ebd8e
    • Thomas Haller's avatar
      manager: fail early from nm_manager_deactivate_connection() · a0a36d55
      Thomas Haller authored
      Drop the @success variable, and just return on error right away.
      a0a36d55
    • Thomas Haller's avatar
      device: always disconnect in nm_device_disconnect_active_connection() · e1b0451d
      Thomas Haller authored
      Previously, if @active referenced a device but was not currently queued
      or the current activation request, nothing was done.
      
      Now, in such a case still call nm_active_connection_set_state_fail().
      Note that nm_active_connection_set_state_fail() has no effects on
      active-connections that are already in disconnected state (which
      we would expect by such an active connection). Likely there is no
      visible change here, but it feels more correct to ensure the active
      connection is always failed.
      e1b0451d
    • Thomas Haller's avatar
      device: use correct active-connection's state-change reason in... · 71a090c9
      Thomas Haller authored
      device: use correct active-connection's state-change reason in nm_device_disconnect_active_connection()
      
      It just makes more sense, to let the caller decide on the reason.
      71a090c9
    • Thomas Haller's avatar
      8f360197
    • Thomas Haller's avatar
      device: pass active-connection's state-change reason to _clear_queued_act_request() · fe5f5f7a
      Thomas Haller authored
      No change in behavior, yet.
      fe5f5f7a
    • Thomas Haller's avatar
      device: add state-change reason argument to nm_device_disconnect_active_connection() · 461bf7aa
      Thomas Haller authored
      nm_device_disconnect_active_connection() is generally useful and a prefered
      form to fail an active connection. The device's state-change reason is important,
      so it needs to be injected.
      461bf7aa
    • Thomas Haller's avatar
      keep-alive: rename nm_keep_alive_sink() to nm_keep_alive_arm() · 7578e59b
      Thomas Haller authored
      The names "floating" and "sink()" are well known and good.
      
      However, "disarm()" seems the best name for the counterpart operation,
      better than "float()", "unsink()", or "freeze()".
      
      Since we have "nm_keep_alive_disarm()", for consitency rename
      
        - "floating" -> (not) "armed"
        - "sink()"   -> "arm()"
      7578e59b
    • Thomas Haller's avatar
      keep-alive: drop "floating" argument from nm_keep_alive_new() · a1e811b4
      Thomas Haller authored
      All callers only want to create floating instances at first.
      Also, it seems to make generally more sense this way: you create
      a floating instance, set it up, and then arm it.
      
      This simplifies nm_keep_alive_new(), which previously was adding
      additional code that wasn't accessible via plain g_object_new().
      a1e811b4
    • Thomas Haller's avatar
      keep-alive: add nm_keep_alive_disarm() to silence notifications once we disconnect · 15033be1
      Thomas Haller authored
      The NMKeepAlive instance is useful to find out when we should disconnect.
      The moment when we start disconnecting, we don't care about it anymore.
      
      Add a nm_keep_alive_disarm() function to suppress property change events about
      the alive state, after that point. Emitting further events from that point
      on is only confusing.
      
      Yes, this means, a NMKeepAlive instance shall not be re-used for
      multiple purposes. Create a separate keep-alive instace for each target
      that should be guarded.
      
      Also, once disarmed, we can release all resources that the NMKeepAlive instance
      was holding until now.
      15033be1
    • Thomas Haller's avatar
  2. 06 Dec, 2018 1 commit
  3. 04 Dec, 2018 2 commits
    • Beniamino Galvani's avatar
      core: avoid assertion when removing devices · 92e57ab2
      Beniamino Galvani authored
      remove_device() is also called when the device has no longer a valid
      ifindex and so device_is_wake_on_lan() must do an extra check to avoid
      the following assertion:
      
       nmp_cache_lookup_entry_link: assertion 'ifindex > 0' failed
      
       0  _g_log_abort () from target:/lib64/libglib-2.0.so.0
       1  g_logv () from target:/lib64/libglib-2.0.so.0
       2  g_log () from target:/lib64/libglib-2.0.so.0
       3  nmp_cache_lookup_entry_link (cache=0xb858f0, ifindex=ifindex@entry=0) at ../src/platform/nmp-object.c:1713
       4  nmp_cache_lookup_link (cache=<optimized out>, ifindex=ifindex@entry=0) at ../src/platform/nmp-object.c:1728
       5  nm_platform_link_get_obj (self=self@entry=0xb85840, ifindex=ifindex@entry=0, visible_only=visible_only@entry=1) at ../src/platform/nm-platform.c:759
       6  nm_platform_link_get (self=self@entry=0xb85840, ifindex=ifindex@entry=0) at ../src/platform/nm-platform.c:784
       7  nm_platform_link_get_type (self=self@entry=0xb85840, ifindex=ifindex@entry=0) at ../src/platform/nm-platform.c:1065
       8  link_get_wake_on_lan (platform=0xb85840, ifindex=0) at ../src/platform/nm-linux-platform.c:6963
       9  nm_platform_link_get_wake_on_lan (self=self@entry=0xb85840, ifindex=0) at ../src/platform/nm-platform.c:1705
       10 device_is_wake_on_lan (platform=0xb85840, device=<optimized out>) at ../src/nm-manager.c:1617
       11 remove_device (self=0xbd0060, device=<optimized out>, device@entry=0xd298c0, quitting=quitting@entry=0, allow_unmanage=allow_unmanage@entry=1)
       12 device_removed_cb (device=0xd298c0, user_data=0xbd0060) at ../src/nm-manager.c:1698
       13 _g_closure_invoke_va () from target:/lib64/libgobject-2.0.so.0
       14 g_signal_emit_valist () from target:/lib64/libgobject-2.0.so.0
       15 g_signal_emit () from target:/lib64/libgobject-2.0.so.0
       16 available_connections_check_delete_unrealized_on_idle (user_data=0xd298c0) at ../src/devices/nm-device.c:4446
      
      Fixes: ca3bbede
      92e57ab2
    • Thomas Haller's avatar
      6ba9f47c
  4. 03 Dec, 2018 16 commits
    • Thomas Haller's avatar
      core: avoid calling platform code with invalid ifindex · d45eed44
      Thomas Haller authored
      Since commit 945c904f "platform: assert against valid ifindex and
      remove duplicate assertions", it is no longer allowed to call certain
      platform functions with invalid ifindex.
      
      These trigger now an assertion. Note that the assertion is merely a
      g_return_val_if_fail(), hence in non-debug mode, this does not lead to
      a crash.
      
      Fixes: 945c904f
      d45eed44
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      device/shared: set ANDROID_METERED option 43 for shared connections · 1a2e767f
      Thomas Haller authored
      The problem is that updating the metered value of a shared connection is
      not implemented. The user needs to fully reactivate the profile for changes
      to take effect.
      
      That is unfortunate, especially because reapplying the route metric
      works in other other cases.
      1a2e767f
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      dnsmasq: refactor construction of command line options in create_dm_cmd_line() · 4419dbed
      Thomas Haller authored
      Having a NMCmdLine implementation here is wrong.
      
      For one, it local to nm-dnsmasq-manager.c and not reusable.
      If there is anything of value in such an implementation, then it should
      possibly also be useful at other places that create command line
      arguments.
      
      Note that in the end, command line arguments are just strv arrays.
      There are different ways how to construct that strv array. For example,
      do we need to clone the strings that we add? How to do that most
      elegantly and efficiently? The previous implementation for example used a
      GStringChunk for that (quite creative!). The point is, there are pros and
      cons about how to create strv arrays. But constructing command line options
      shouldn't be abstracted in a NMCmdLine API. It should use a suitable API
      for creating an strv array. Otherwise, it's too much abstraction.
      
      Drop NMCmdLine and use GPtrArray directly. Together with a few helper
      functions nm_strv_ptrarray_*() that is our preferred way to create such
      strv arrays. Is it perfect? No, we still g_strdup() static strings.
      That could be optimized. But then we would want an optimized API for
      constructing strv arrays, not NMCmdLine.
      4419dbed
    • Thomas Haller's avatar
      shared: add nm_strv_ptrarray_*() helpers · c54a2ed8
      Thomas Haller authored
      These are simple macros/functions that append a heap-allocated string to
      a GPtrArray. It is intended for a GPtrArray which takes ownership of the
      strings (meaning, it has a g_free() GDestroyNotify).
      c54a2ed8
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      platform: don't consult cache before invoking netlink operation · f9414228
      Thomas Haller authored
      Checking whether the link exists in the cache, before talking to kernel
      serves no purpose.
      
      - in all cases, the caller already has a good indication that the link
        in fact exists. That is, because the caller makes decisions on what to
        do, based on what platform told it earlier. Thus, the check usually succeeds
        anyway.
      
      - in the unexpected case it doesn't succeed, we
      
        - should not silently return without logging at least a message
      
        - we possibly still want to send the netlink message to kernel,
          just to have it fail. Note that the ifindex is indeed the identifier
          for the link, so there is no danger of accidentally killing the
          wrong link.
          Well, theoretically there is, because the kernel's ifindex counter can
          wrap or get reused when moving links between namespaces. But checking
          the cache would not protect against that anyway! Worst case, the cache
          would already have the impostor link and would not prevent from doing
          the wrong thing. After all, they do have the same identifier, so how
          would we know that this is in fact a different link?
      f9414228
    • Thomas Haller's avatar
      platform: assert against valid ifindex and remove duplicate assertions · 945c904f
      Thomas Haller authored
      We want that all code paths assert strictly and gracefully.
      
      That means, if we have function nm_platform_link_get() which calls
      nm_platform_link_get_obj(), then we don't need to assert the same things
      twice. Don't have the calling function assert itself, if it is obvious
      that the first thing that it does, is calling a function that itself
      asserts the same conditions.
      
      On the other hand, it simply indicates a bug passing a non-positive
      ifindex to any of these platform functions. No longer let
      nm_platform_link_get_obj() handle negative ifindex gracefully. Instead,
      let it directly pass it to nmp_cache_lookup_link(), which eventually
      does a g_return_val_if_fail() check. This quite possible enables
      assertions on a lot of code paths. But note that g_return_val_if_fail()
      is graceful and does not lead to a crash (unless G_DEBUG=fatal-criticals
      is set for debugging).
      945c904f
    • Thomas Haller's avatar
      platform/tests: improve nmtstp_link_delete() for deleting links · da39a0ad
      Thomas Haller authored
      nm_platform_link_delete() will soon assert against positive ifindex
      argument.
      
          nm_platform_link_delete (NM_PLATFORM_GET, nm_platform_link_get_ifindex (NM_PLATFORM_GET, DEVICE_NAME));
      
      will result in an assertion, if the link does not exist.
      
      Extend nmtstp_link_delete() to gracefully skip deleting the link
      so that it can be used in such situations.
      
      Also, rename nmtstp_link_del() to nmtstp_link_delete(), because it's
      closer to nm_platform_link_delete().
      da39a0ad
    • Thomas Haller's avatar
      platform: move assertion from nm_platform_link_get() to nm_platform_link_get_obj() · 1c7b747f
      Thomas Haller authored
      We want to assert for valid input arguments, but we don't want
      multiple assertions for the same.
      
      Move the assertion from nm_platform_link_get() to
      nm_platform_link_get_obj().
      
      That way, nm_platform_link_get_obj() also checks the input arguments.
      At the same time, nm_platform_link_get() gets simpler and still does
      the same amount of assertions.
      1c7b747f
    • Thomas Haller's avatar
      platform: let nmp_cache_lookup_link_full() prefer visible links · f47f9e39
      Thomas Haller authored
      In nmp_cache_lookup_link_full(), we may have multiple candidates that match.
      Continue searching, until we find a visible one. That way, visible results
      are preferred.
      
      Note that for links, nmp_object_is_visible() checks whether the link is
      visible in netlink (instead of only udev).
      f47f9e39
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      keyfile: add helper functions to record loaded UUID files · 3fc5765e
      Thomas Haller authored
      This code will be used later.
      
      We want to remember which keyfiles are currently loaded (or hidden).
      
      With the addition or multiple keyfile directories (soon), there are
      two cases where this matters:
      
       - if there are multiple keyfiles which reference the same UUID,
         we can only load one of them. That is already a problem today
         with only one keyfile directory, where multiple files can reference
         the same UUID.
         The implementation will pick the file based on priorities (like
         the file modification date). However, the user may call explicitly
         call `nmcli connection load`. In that case, we cannot reload
         all files to find out whether the to be loaded file is hidden
         according to the defined priorities. We cannot do that, because we
         must not make decisions based on files on disk, which we are not told
         to reload. So, during a `nmcli connection load` we must look at
         unrelated files, to determine how to load the file.
         Instead, we do allow the user to load any file, even if it would be
         shadowed by other files. When we do that, we may want to persist which
         file is currently loaded, so that a service restart and a `nmcli connection
         reload` does not undo the load again. This can be later later be solved by
         writing a symlink
      
             "/var/run/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile"
      
         which targets the currently active file.
      
       - if a profile was loaded from read-only persistant storage, the user
         may still delete the profile. We also need to remember the deletion
         of the file. That will be achieved by symlinking "/dev/null" as
         "/etc/NetworkManager/system-connections/.loaded-$UUID.nmkeyfile".
      
      Add helper functions to read and write these symlinks.
      3fc5765e
    • Thomas Haller's avatar
      f7de10ac
    • Thomas Haller's avatar
      keyfile/tests: add tests for ignoring keyfile filenames · 4d8ce80e
      Thomas Haller authored
      In particular, have a full path (with slashes), and a filename
      with trailing slash (a directory).
      4d8ce80e
  5. 01 Dec, 2018 7 commits