1. 09 Dec, 2018 3 commits
    • 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 13 commits
  6. 30 Nov, 2018 1 commit
    • Beniamino Galvani's avatar
      cli: avoid crash on device disconnect · cf1126f6
      Beniamino Galvani authored
      When nm_device_disconnect_async() returns, the device could be still
      in DEACTIVATING state, and so we also register to device-state signal
      notifications to know when the device state goes to DISCONNECTED.
      
      Sometimes it happens that the device state goes to DISCONNECTED before
      nm_device_disconnect_async() returns. In this case the signal handler
      exits the main loop and then the callback for disconnect_async() is
      executed anyway because it was already dispatched, leading to an
      invalid memory access.
      
      To avoid this we should cancel nm_device_disconnect_async() when we
      are quitting the main loop.
      
      Reproducer:
        nmcli connection add type team ifname t1 con-name t1
        nmcli connection up t1
        nmcli device disconnect t1 & nmcli device delete t1
      
      Crash example:
       ==14955==ERROR: AddressSanitizer: SEGV on unknown address 0xffffffff0000000b (pc 0x7f128c8ba3dd bp 0x0000004be080 sp 0x7ffcda7dc6e0 T0)
       ==14955==The signal is caused by a READ memory access.
          0 0x7f128c8ba3dc in g_string_truncate (/lib64/libglib-2.0.so.0+0x713dc)
          1 0x7f128c8bb4bb in g_string_printf (/lib64/libglib-2.0.so.0+0x724bb)
          2 0x45bdfa in disconnect_device_cb clients/cli/devices.c:2321
          3 0x7f128ca3d1a9 in g_simple_async_result_complete /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gsimpleasyncresult.c:802
          4 0x7f128cf85d0e in device_disconnect_cb libnm/nm-device.c:2354
          5 0x7f128ca4ff73 in g_task_return_now /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1148
          6 0x7f128ca508d5 in g_task_return /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1206
          7 0x7f128ca8ecfc in reply_cb /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gdbusproxy.c:2586
          8 0x7f128ca4ff73 in g_task_return_now /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1148
          9 0x7f128ca508d5 in g_task_return /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1206
          10 0x7f128ca83440 in g_dbus_connection_call_done /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gdbusconnection.c:5713
          11 0x7f128ca4ff73 in g_task_return_now /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1148
          12 0x7f128ca4ffac in complete_in_idle_cb /usr/src/debug/glib2-2.58.1-1.fc29.x86_64/gio/gtask.c:1162
          13 0x7f128c893b7a in g_idle_dispatch gmain.c:5620
          14 0x7f128c89726c in g_main_dispatch gmain.c:3182
          15 0x7f128c897637 in g_main_context_iterate gmain.c:3920
          16 0x7f128c897961 in g_main_loop_run (/lib64/libglib-2.0.so.0+0x4e961)
          17 0x473afb in main clients/cli/nmcli.c:1067
          18 0x7f128c6a1412 in __libc_start_main (/lib64/libc.so.6+0x24412)
          19 0x416c39 in _start (/usr/bin/nmcli+0x416c39)
      
      https://github.com/NetworkManager/NetworkManager/pull/254
      https://bugzilla.redhat.com/show_bug.cgi?id=1546061
      cf1126f6
  7. 29 Nov, 2018 4 commits