1. 15 Oct, 2019 7 commits
  2. 14 Oct, 2019 3 commits
    • Thomas Haller's avatar
      core: don't use pointer value for pending action string in active-connection · 10c63f16
      Thomas Haller authored
      The pending action gets logged. We should not log plain pointer
      values because they may be used to defeat ASLR.
      
      Instead, construct the pending action using the "version_id". This
      number is also unique, and suits sufficiently well. With debug logging
      you can still grep the log for the corresponding active-connection (and
      anyway it's obvious from the context).
      10c63f16
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      device: don't delay startup complete for pending-actions "autoconf", "dhcp4" and "dhcp6" · 1e520641
      Thomas Haller authored
      These "pending-actions" only have one purpose: to mark the device
      as busy and thereby delay "startup complete" to be reached. That
      in turn delays "NetworkManager-wait-online" service.
      
      Of course, "NetworkManager-wait-online" waits for some form of readiness
      and is not extensively configurable (e.g. you cannot exclude devices from
      being waited). However, the intent is to wait that all devices are "settled".
      That means among others, that the timeouts waiting for carrier and Wi-Fi scan
      results passed, and devices either don't have a connection profile to autoactivate,
      or they autoactivated profiles and are in state "connected".
      
      A major point here is that the device is considered ready, once it
      reaches the state "connected". Note that if you configure both IPv4 and
      IPv6 addressing modes, than "ipv4.may-fail=yes" and "ipv6.may-fail=yes"
      means, that the device is considered fully activated once one address
      family completes. Again, this is not very configurable, but by setting
      "ipv6.may-fail=no", you can require that the device has indeed IPv6
      addressing completed.
      
      Now, the determining factor for declaring "startup complete" is whether the
      device is in state "connected". That may or may not mean that DHCPv4,
      autoconf or DHCPv6 completed, as it depends on a overall state of the
      device. So, it is wrong to have distinct pending actions for these operations.
      
      Remove them.
      
      This fixes that we wrongly would wait too long before declaring startup
      complete. But it is also a change in behavior.
      1e520641
  3. 13 Oct, 2019 2 commits
  4. 12 Oct, 2019 2 commits
    • Thomas Haller's avatar
      clients/tests: fix handling timeout for asynchronous jobs · b6725a59
      Thomas Haller authored
      Fixes: bb4b7495 ('clients/tests: don't wait for first job before scheduling parallel jobs')
      b6725a59
    • Thomas Haller's avatar
      clients/tests: don't wait for first job before scheduling parallel jobs · bb4b7495
      Thomas Haller authored
      Previously, the test would kick off 15 processes in parallel, but
      the first job in the queue would block more processes from being
      started.
      
      That is, async_start() would only start 15 processes, but since none of
      them were reaped before async_wait() was called, no more than 15 jobs
      were running during the start phase. That is not a real issue, because
      the start phase is non-blocking and queues all the jobs quickly. It's
      not really expected that during that time many processes already completed.
      Anyway, this was a bit ugly.
      
      The bigger problem is that async_wait() would always block for the
      first job to complete, before starting more processes. That means,
      if the first job in the queue takes unusually long, then this blocks
      other processes from getting reaped and new processes from being
      started.
      
      Instead, don't block only one one jobs, but poll them in turn for a
      short amount of time. Whichever process exits first will be completed
      and more jobs will be started.
      
      In fact, in the current setup it's hard to notice any difference,
      because all nmcli invocations take about the same time and are
      relatively fast. That this approach parallelizes better can be seen
      when the runtime of jobs varies stronger (and some invocations take
      a notably longer time). As we later want to run nmcli under valgrind,
      this probably will make a difference.
      
      An alternative would be not to poll()/wait() for child processes,
      but somehow get notified. For example, we could use a GMainContext
      and watch child processes. But that's probably more complicated
      to do, so let's keep the naive approach with polling.
      bb4b7495
  5. 10 Oct, 2019 9 commits
  6. 09 Oct, 2019 3 commits
  7. 08 Oct, 2019 1 commit
  8. 07 Oct, 2019 1 commit
  9. 04 Oct, 2019 2 commits
  10. 03 Oct, 2019 5 commits
    • Thomas Haller's avatar
      5a24ad53
    • Ilya Shipitsin's avatar
      src/devices/nm-device.c: resolve possible null pointer dereference · e8588d0c
      Ilya Shipitsin authored
      found by cppcheck
      
      [src/devices/nm-device.c:3032] -> [src/devices/nm-device.c:3025]: (warning) Either the condition '!handle' is redundant or there is possible null pointer dereference: handle.
      
      https://github.com/NetworkManager/NetworkManager/pull/352
      e8588d0c
    • Thomas Haller's avatar
      adac530d
    • Thomas Haller's avatar
      libnm: deprecate nm_client_check_connectivity() in 1.22 · f45aeba4
      Thomas Haller authored
      The previous commit marks all synchronous libnm API as deprecated.
      In practice, the macro _NM_DEPRECATED_SYNC_METHOD expands to
      nothing, because there is no immediate urgency to force users
      to migrate.
      
      However nm_client_check_connectivity() is especially bad: it
      makes a synchronous call and then updates the content of the
      cache artificially. Usually, NMClient's cache of D-Bus objects
      is only updated by "PropertiesChanged" D-Bus signals.
      nm_client_check_connectivity() instead will act on the response to
      the "CheckConnectivity" D-Bus call -- a response that is picked
      out of order from the ordered sequence of messages --  and will
      update the cache instead of honoring the usual "PropertiesChanged"
      signal.
      
      I think such behavior is fundamentally broken. For a trivial property like
      NM_CLIENT_CONNECTIVITY such behavior is odd at best. Note how applying
      this approach to other functions (like nm_client_deactivate_connection(),
      which would affect a much larger state) would not be feasible.
      
      I also imagine it to be complicate to preserve this behavior when
      reworking libnm, as I plan to do.
      
      See also commit b799de28 ('libnm: update property in the manager
      after connectivity check'), which introduced this behavior to "fix"
      bgo#784629.
      f45aeba4
    • Thomas Haller's avatar
      libnm: deprecate synchronous/blocking API in libnm · e90684a1
      Thomas Haller authored
      Note that D-Bus is fundamentally asynchronous. Doing blocking calls
      on top of D-Bus is odd, especially for libnm's NMClient. That is because
      NMClient essentially is a client-side cache of the objects from the D-Bus
      interface. This cache should be filled exclusively by (asynchronous) D-Bus
      events (PropertiesChanged). So, making a blocking D-Bus call means to wait
      for a response and return it, while queuing all messages that are received
      in the meantime.
      Basically there are three ways how a synchronous API on NMClient could behave:
      
       1) the call just calls g_dbus_connection_call_sync(). This means
          that libnm sends a D-Bus request via GDBusConnection, and blockingly
          waits for the response. All D-Bus messages that get received in the
          meantime are queued in the GMainContext that belongs to NMClient.
          That means, none of these D-Bus events are processed until we
          iterate the GMainContext after the call returns. The effect is,
          that NMClient (and all cached objects in there) are unaffected by
          the D-Bus request.
          Most of the synchronous API calls in libnm are of this kind.
          The problem is that the strict ordering of D-Bus events gets
          violated.
          For some API this is not an immediate problem. Take for example
          nm_device_wifi_request_scan(). The call merely blockingly tells
          NetworkManager to start scanning, but since NetworkManager's D-Bus
          API does not directly expose any state that tells whether we are
          currently scanning, this out of order processing of the D-Bus
          request is a small issue.
          The problem is more obvious for nm_client_networking_set_enabled().
          After calling it, NM_CLIENT_NETWORKING_ENABLED is still unaffected
          and unchanged, because the PropertiesChanged signal from D-Bus
          is not yet processed.
          This means, while you make such a blocking call, NMClient's state
          does not change. But usually you perform the synchronous call
          to change some state. In this form, the blocking call is not useful,
          because NMClient only changes the state after iterating the GMainContext,
          and not after the blocking call returns.
      
       2) like 1), but after making the blocking g_dbus_connection_call_sync(),
          update the NMClient cache artificially. This is what
          nm_manager_check_connectivity() does, to "fix" bgo#784629.
          This also has the problem of out-of-order events, but it kinda
          solves the problem of not changing the state during the blocking
          call. But it does so by hacking the state of the cache. I think
          this is really wrong because the state should only be updated from
          the ordered stream of D-Bus messages (PropertiesChanged signal and
          similar). When libnm decides to modify the state, there may be already
          D-Bus messages queued that affect this very state.
      
       3) instead of calling g_dbus_connection_call_sync(), use the
          asynchronous g_dbus_connection_call(). If we would use a sepaate
          GMainContext for all D-Bus related calls, we could ensure that
          while we block for the response, we iterate that internal main context.
          This might be nice, because all events are processed in order and
          after the blocking call returns, the NMClient state is up to date.
          The are problems however: current blocking API does not do this,
          so it's a significant change in behavior. Also, it might be
          unexpected to the user that during the blocking call the entire
          content of NMClient's cache might change and all pointers to the
          cache might be invalidated. Also, of course NMClient would invoke
          signals for all the changes that happen.
          Another problem is that this would be more effort to implement
          and it involves a small performance overhead for all D-Bus related
          calls (because we have to serialize all events in an internal
          GMainContext first and then invoke them on the caller's context).
          Also, if the users wants this behavior, they could implement it themself
          by running libnm in their own GMainContext. Note that libnm might
          have bugs to make that really working, but that should be fixed
          instead of adding such synchrnous API behavior.
      
      Read also [1], for why blocking calls are wrong.
      
      [1] https://smcv.pseudorandom.co.uk/2008/11/nonblocking/
      
      So, all possible behaviors for synchronous API have severe behavioural
      issues.  Mark all this API as deprecated. Also, this serves the purpose of
      identifying blocking D-Bus calls in libnm.
      
      Note that "deprecated" here does not really mean that the API is going
      to be removed. We don't break API. The user may:
      
        - continue to use this API. It's deprecated, awkward and discouraged,
          but if it works, by all means use it.
      
        - use asynchronous API. That's the only sensible way to use D-Bus.
          If libnm lacks a certain asynchronous counterpart, it should be
          added.
      
        - use GDBusConnection directly. There really isn't anything wrong
          with D-Bus or GDBusConnection. This deprecated API is just a wrapper
          around g_dbus_connection_call_sync(). You may call it directly
          without feeling dirty.
      
      ---
      
      The only other remainging API is the synchronous GInitable call for
      NMClient. That is an entirely separate beast and not particularly
      wrong (from an API point of view).
      
      Note that synchronous API in NMSecretAgentOld, NMVpnPluginOld and
      NMVpnServicePlugin as not deprecated here. These types are not part
      of the D-Bus cache and while they have similar issues, it's less severe
      because they have less state.
      e90684a1
  11. 02 Oct, 2019 5 commits
    • Thomas Haller's avatar
      checkpatch,gitlab-ci: let checkpatch script compare against latest upstream master · 3019648b
      Thomas Haller authored
      When opening a merge request from a fork of NetworkManager, then the
      pipeline runs with the a checkout of the fork. That means, checkpatch
      would compare the branch against "master" (or "nm-x-y" stable branches)
      of the fork, instead of upstream.
      
      That doesn't seem too useful. Instead, also add upstream NetworkManager
      as git remote, fetch the branches, and use the branches from there as
      base for checkpatch.
      
      #255
      3019648b
    • Thomas Haller's avatar
      cli: translate overview output of nmcli · 10e8f7fd
      Thomas Haller authored
      Note the "to" in the output:
      
        $ LANG=de_DE.UTF-8 nmcli
        eth0: verbunden to Wired Connection 1
              "Intel Ethernet"
        ...
      
      #246
      10e8f7fd
    • Thomas Haller's avatar
      all: unify format of our Copyright source code comments · 3b69f021
      Thomas Haller authored
      ```bash
      
      readarray -d '' FILES < <(
        git ls-files -z \
          ':(exclude)po' \
          ':(exclude)shared/c-rbtree' \
          ':(exclude)shared/c-list' \
          ':(exclude)shared/c-siphash' \
          ':(exclude)shared/c-stdaux' \
          ':(exclude)shared/n-acd' \
          ':(exclude)shared/n-dhcp4' \
          ':(exclude)src/systemd/src' \
          ':(exclude)shared/systemd/src' \
          ':(exclude)m4' \
          ':(exclude)COPYING*'
        )
      
      sed \
        -e 's/^\(--\|#\| \*\) *\(([cC]) *\)\?Copyright \+\(\(([cC])\) \+\)\?\(\(20\|19\)[0-9][0-9]\) *[-–] *\(\(20\|19\)[0-9][0-9]\) \+\([^ ].*\)$/\1 C1pyright#\5 - \7#\9/' \
        -e 's/^\(--\|#\| \*\) *\(([cC]) *\)\?Copyright \+\(\(([cC])\) \+\)\?\(\(20\|19\)[0-9][0-9]\) *[,] *\(\(20\|19\)[0-9][0-9]\) \+\([^ ].*\)$/\1 C2pyright#\5, \7#\9/' \
        -e 's/^\(--\|#\| \*\) *\(([cC]) *\)\?Copyright \+\(\(([cC])\) \+\)\?\(\(20\|19\)[0-9][0-9]\) \+\([^ ].*\)$/\1 C3pyright#\5#\7/' \
        -e 's/^Copyright \(\(20\|19\)[0-9][0-9]\) \+\([^ ].*\)$/C4pyright#\1#\3/' \
        -i \
        "${FILES[@]}"
      
      echo ">>> untouched Copyright lines"
      git grep Copyright "${FILES[@]}"
      
      echo ">>> Copyright lines with unusual extra"
      git grep '\<C[0-9]pyright#' "${FILES[@]}" | grep -i reserved
      
      sed \
        -e 's/\<C[0-9]pyright#\([^#]*\)#\(.*\)$/Copyright (C) \1 \2/' \
        -i \
        "${FILES[@]}"
      
      ```
      
      !298
      3b69f021
    • Lubomir Rintel's avatar
      bluetooth: don't set the ifindex after the device has been activated · a5ca504b
      Lubomir Rintel authored
      The Bluetooth DUN device's NMModem would signal the reset of ifindex to zero
      when it's disconnected and the NMDeviceBt would accordingly update the
      bluetooth device's ip ifindex. This is not okay since commit ab457830
      ('device: refactor nm_device_set_ip_ifindex() and set_ip_iface()') which,
      although claiming to be a refactoring, made such use of
      nm_device_set_ip_ifindex() illegal. Resetting the ifindex is anyway not
      necessary, since it's taken care of _cleanup_generic_post().
      
      Let's leave the ifindex alone once the device is activated, in a manner
      analogous to what NMDeviceModem.
      
      Fixes: ab457830 ('device: refactor nm_device_set_ip_ifindex() and set_ip_iface()')
      Fixes: 78ca2a70 ('device: don't set invalid ip-iface'):
      a5ca504b
    • Lubomir Rintel's avatar
      contrib: add a Bluetooth DUN modem emulator · 8d352b5a
      Lubomir Rintel authored
      Useful for quickly testing Bluetooth DUN support. Duplicates some
      modemu.pl logic, but hey...
      
      !297
      8d352b5a