1. 16 Oct, 2019 15 commits
  2. 03 Oct, 2019 1 commit
    • 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
  3. 02 Oct, 2019 1 commit
    • 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
  4. 17 Sep, 2019 1 commit
  5. 10 Sep, 2019 1 commit
  6. 25 Jul, 2019 1 commit
    • Thomas Haller's avatar
      core,libnm: add AddConnection2() D-Bus API to block autoconnect from the start · 22c8721f
      Thomas Haller authored
      It should be possible to add a profile with autoconnect blocked form the
      start. Update2() has a %NM_SETTINGS_UPDATE2_FLAG_BLOCK_AUTOCONNECT flag to
      block autoconnect, and so we need something similar when adding a connection.
      
      As the existing AddConnection() and AddConnectionUnsaved() API is not
      extensible, add AddConnection2() that has flags and room for additional
      arguments.
      
      Then add and implement the new flag %NM_SETTINGS_ADD_CONNECTION2_FLAG_BLOCK_AUTOCONNECT
      for AddConnection2().
      
      Note that libnm's nm_client_add_connection2() API can completely replace
      the existing nm_client_add_connection_async() call. In particular, it
      will automatically prefer to call the D-Bus methods AddConnection() and
      AddConnectionUnsaved(), in order to work with server versions older than
      1.20. The purpose of this is that when upgrading the package, the
      running NetworkManager might still be older than the installed libnm.
      Anyway, so since nm_client_add_connection2_finish() also has a result
      output, the caller needs to decide whether he cares about that result.
      Hence it has an argument ignore_out_result, which allows to fallback to
      the old API. One might argue that a caller who doesn't care about the
      output results while still wanting to be backward compatible, should
      itself choose to call nm_client_add_connection_async() or
      nm_client_add_connection2(). But instead, it's more convenient if the
      new function can fully replace the old one, so that the caller does not
      need to switch which start/finish method to call.
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1677068
      22c8721f
  7. 22 Jul, 2019 1 commit
  8. 11 Jun, 2019 1 commit
    • Thomas Haller's avatar
      all: drop emacs file variables from source files · c0e075c9
      Thomas Haller authored
      We no longer add these. If you use Emacs, configure it yourself.
      
      Also, due to our "smart-tab" usage the editor anyway does a subpar
      job handling our tabs. However, on the upside every user can choose
      whatever tab-width he/she prefers. If "smart-tabs" are used properly
      (like we do), every tab-width will work.
      
      No manual changes, just ran commands:
      
          F=($(git grep -l -e '-\*-'))
          sed '1 { /\/\* *-\*-  *[mM]ode.*\*\/$/d }'     -i "${F[@]}"
          sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
      
      Check remaining lines with:
      
          git grep -e '-\*-'
      
      The ultimate purpose of this is to cleanup our files and eventually use
      SPDX license identifiers. For that, first get rid of the boilerplate lines.
      c0e075c9
  9. 15 Mar, 2019 2 commits
  10. 07 Mar, 2019 2 commits
  11. 22 Feb, 2019 1 commit
  12. 12 Feb, 2019 1 commit
  13. 01 Feb, 2019 2 commits
    • Thomas Haller's avatar
      wifi-p2p: rename Wi-Fi P2P · 09090f26
      Thomas Haller authored
      After renaming the files, also rename all the content
      to follow the "Wi-Fi P2P" naming scheme.
      09090f26
    • Thomas Haller's avatar
      wifi-p2p: rename files for consistent Wi-Fi P2P naming · 0420fa1f
      Thomas Haller authored
      We named the types inconsistently:
      
        - "p2p-wireless" ("libnm-core/nm-setting-p2p-wireless.h")
      
        - "p2p" ("libnm/nm-p2p-peer.h")
      
        - "p2p-wifi" ("src/devices/wifi/nm-device-p2p-wifi.h")
      
      It seems to me, "libnm/nm-p2p-peer.h" should be qualified with a "Wi-Fi"
      specific name. It's not just peer-to-peer, it's Wi-Fi P2P.
      Yes, there is an inconsistency now, because there is already
      "libnm/nm-access-point.h".
      
      It seems to me (from looking at the internet), that the name "Wi-Fi P2P"
      is more common than "P2P Wi-Fi" -- although both are used. There is also
      the name "Wi-Fi Direct". But it's not clear which name should be
      preferred here, so stick to "Wi-Fi P2P".
      
      In this first commit only rename the files. The following commit will
      rename the content.
      0420fa1f
  14. 27 Jan, 2019 2 commits
  15. 14 Jan, 2019 1 commit
    • Thomas Haller's avatar
      all: return output dictionary from "AddAndActivate2" · fbb038af
      Thomas Haller authored
      Add a "a{sv}" output argument to "AddAndActivate2" D-Bus API.
      "AddAndActivate2" replaces "AddAndActivate" with more options.
      It also has a dictionary argument to be forward compatible so that we
      hopefully won't need an "AddAndActivate3". However, it lacked a similar
      output dictionary. Add it for future extensibility. I think this is
      really to workaround a shortcoming of D-Bus, which does provide strong
      typing and type information about its API, but does not allow to extend
      an existing API in a backward compatible manner. So we either resort to
      Method(), Method2(), Method3() variants, or a catch-all variant with a
      generic "a{sv}" input/output argument.
      
      In libnm, rename "nm_client_add_and_activate_connection_options()" to
      "nm_client_add_and_activate_connection2()". I think libnm API should have
      an obvious correspondence with D-Bus API. Or stated differently, if
      "AddAndActivateOptions" would be a better name, then the D-Bus API should
      be renamed. We should prefer one name over the other, but regardless
      of which is preferred, the naming for D-Bus and libnm API should
      correspond.
      
      In this case, I do think that AddAndActivate2() is a better name than
      AddAndActivateOptions(). Hence I rename the libnm API.
      
      Also, unless necessary, let libnm still call "AddAndActivate" instead of
      "AddAndActivate2". Our backward compatibility works the way that libnm
      requires a server version at least as new as itself. As such, libnm
      theoretically could assume that server version is new enough to support
      "AddAndActivate2" and could always use the more powerful variant.
      However, we don't need to break compatibility intentionally and for
      little gain. Here, it's easy to let libnm also handle old server API, by
      continuing to use "AddAndActivate" for nm_client_add_and_activate_connection().
      Note that during package update, we don't restart the currently running
      NetworkManager instance. In such a scenario, it can easily happen that
      nmcli/libnm is newer than the server version. Let's try a bit harder
      to not break that.
      
      Changes as discussed in [1].
      
      [1] !37 (comment 79876)
      fbb038af
  16. 19 Nov, 2018 2 commits
    • Thomas Haller's avatar
      all: rename "bind" option for AddAndActivateConnection2 to "bind-activation" · 7420ae83
      Thomas Haller authored
      "bind" specifically binds the lifetime of the activation (NMActiveConnection).
      In combination with "persist=volatile", the lifetime of the NMSettingsConnection
      is indirectly bound to the NMActiveConnection. But still these concepts make sense
      independently.
      In the future, it may make sense to also bind the lifetime of the NMSettingsConnection
      to the D-Bus client. Hence, rename the option to allow for the distinction.
      
      Also, belatedly fix libnm comment about "bind" only working with
      "persist" "volatile".
      
      Fixes: eb883e34
      7420ae83
    • Thomas Haller's avatar
      libnm: drop "_async" suffix from nm_client_add_and_activate_connection_options() · 26eaca89
      Thomas Haller authored
      Synchronous D-Bus calls seems harmful to me, such API should not be
      added to libnm. As such, all API is by default and preferably "_async".
      
      Don't add an "_async" suffix. While we are not consistent in libnm about
      this, I think for new code we should.
      26eaca89
  17. 17 Nov, 2018 1 commit
  18. 19 Oct, 2018 1 commit
  19. 17 Sep, 2018 1 commit
  20. 06 Aug, 2018 1 commit
  21. 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 guide is instead: don't use these typedefs.
      
      No manual actions, I only ran the bash script:
      
        FILES=($(git ls-files '*.[hc]'))
        sed -i \
            -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
            -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>  /\1   /g' \
            -e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
            "${FILES[@]}"
      e1c7a2b5