1. 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
  2. 18 Apr, 2018 1 commit
  3. 12 Mar, 2018 3 commits
    • Thomas Haller's avatar
      core/dbus: rework D-Bus implementation to use lower layer GDBusConnection API · 297d4985
      Thomas Haller authored
      Previously, we used the generated GDBusInterfaceSkeleton types and glued
      them via the NMExportedObject base class to our NM types. We also used
      GDBusObjectManagerServer.
      
      Don't do that anymore. The resulting code was more complicated despite (or
      because?) using generated classes. It was hard to understand, complex, had
      ordering-issues, and had a runtime and memory overhead.
      
      This patch refactors this entirely and uses the lower layer API GDBusConnection
      directly. It replaces the generated code, GDBusInterfaceSkeleton, and
      GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
      and static descriptor instances of type GDBusInterfaceInfo.
      
      This adds a net plus of more then 1300 lines of hand written code. I claim
      that this implementation is easier to understand. Note that previously we
      also required extensive and complex glue code to bind our objects to the
      generated skeleton objects. Instead, now glue our objects directly to
      GDBusConnection. The result is more immediate and gets rid of layers of
      code in between.
      Now that the D-Bus glue us more under our control, we can address issus and
      bottlenecks better, instead of adding code to bend the generated skeletons
      to our needs.
      
      Note that the current implementation now only supports one D-Bus connection.
      That was effectively the case already, although there were places (and still are)
      where the code pretends it could also support connections from a private socket.
      We dropped private socket support mainly because it was unused, untested and
      buggy, but also because GDBusObjectManagerServer could not export the same
      objects on multiple connections. Now, it would be rather straight forward to
      fix that and re-introduce ObjectManager on each private connection. But this
      commit doesn't do that yet, and the new code intentionally supports only one
      D-Bus connection.
      Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
      succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
      connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
      for the moment. It could be easily extended later, for example with polling whether the
      system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
      supported either -- just like before.
      
      Note how NMDBusManager now implements the ObjectManager D-Bus interface
      directly.
      
      Also, this fixes race issues in the server, by no longer delaying
      PropertiesChanged signals. NMExportedObject would collect changed
      properties and send the signal out in idle_emit_properties_changed()
      on idle. This messes up the ordering of change events w.r.t. other
      signals and events on the bus. Note that not only NMExportedObject
      messed up the ordering. Also the generated code would hook into
      notify() and process change events in and idle handle, exhibiting the
      same ordering issue too.
      No longer do that. PropertiesChanged signals will be sent right away
      by hooking into dispatch_properties_changed(). This means, changing
      a property in quick succession will no longer be combined and is
      guaranteed to emit signals for each individual state. Quite possibly
      we emit now more PropertiesChanged signals then before.
      However, we are now able to group a set of changes by using standard
      g_object_freeze_notify()/g_object_thaw_notify(). We probably should
      make more use of that.
      
      Also, now that our signals are all handled in the right order, we
      might find places where we still emit them in the wrong order. But that
      is then due to the order in which our GObjects emit signals, not due
      to an ill behavior of the D-Bus glue. Possibly we need to identify
      such ordering issues and fix them.
      
      Numbers (for contrib/rpm --without debug on x86_64):
      
      - the patch changes the code size of NetworkManager by
        - 2809360 bytes
        + 2537528 bytes (-9.7%)
      
      - Runtime measurements are harder because there is a large variance
        during testing. In other words, the numbers are not reproducible.
        Currently, the implementation performs no caching of GVariants at all,
        but it would be rather simple to add it, if that turns out to be
        useful.
        Anyway, without strong claim, it seems that the new form tends to
        perform slightly better. That would be no surprise.
      
        $ time (for i in {1..1000}; do nmcli >/dev/null || break; echo -n .;  done)
        - real    1m39.355s
        + real    1m37.432s
      
        $ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects > /dev/null || break; echo -n .; done)
        - real    0m26.843s
        + real    0m25.281s
      
      - Regarding RSS size, just looking at the processes in similar
        conditions, doesn't give a large difference. On my system they
        consume about 19MB RSS. It seems that the new version has a
        slightly smaller RSS size.
        - 19356 RSS
        + 18660 RSS
      297d4985
    • Thomas Haller's avatar
      core: rename "nm-bus-manager.h" to "nm-dbus-manager.h" · a1f37964
      Thomas Haller authored
      The next commit will completely rework NMBusManager and replace
      NMExportedObject by a new type NMDBusObject.
      
      Originally, NMDBusObject was added along NMExportedObject to ease
      the rework and have compilable, intermediate stages of refactoring. Now,
      I think the new name is better, because NMDBusObject is very strongly related
      to the bus manager and the old name NMExportedObject didn't make that
      clear.
      
      I also slighly prefer the name NMDBusObject over NMBusObject, hence
      for consistancy, also rename NMBusManager to NMDBusManager.
      
      This commit only renames the file for a nicer diff in the next commit.
      It does not actually update the type name in sources. That will be done
      later.
      a1f37964
    • Thomas Haller's avatar
      secret-agent: don't use generated NMDBusSecretAgent proxy · 062f86d8
      Thomas Haller authored
      The generated code is really just a thin wrapper around direct
      GDBusProxy calls. GDBusProxy is reasonably convenient to use directly,
      drop this wrapper.
      
      We also don't use a generated wrapper for other cases where
      NetworkManager acts as a D-Bus client. There is no reason to
      do it in this case.
      
      While the nmdbus_*() functions that we were using are small wrappers,
      we also created a NMDBusSecretAgent instance, and hence several other
      functions and symbols are used as well. It's unnecessary.
      
      This saves 8552 bytes for NetworkManager binary (2817944 vs. 2809392
      bytes for contrib/rpm on x86_64).
      062f86d8
  4. 07 Feb, 2018 1 commit
  5. 28 Nov, 2017 1 commit
    • Thomas Haller's avatar
      c-list: re-import latest version of c-list.h from upstream · b6efac9e
      Thomas Haller authored
      Most notably, it renames
        c_list_unlink_init() -> c_list_unlink()
        c_list_unlink() -> c_list_unlink_stale()
      
        $ sed -e 's/\<c_list_unlink\>/c_list_unlink_old/g' \
              -e 's/\<c_list_unlink_init\>/c_list_unlink/g' \
              -e 's/\<c_list_unlink_old\>/c_list_unlink_stale/g' \
              $(git grep -l c_list_unlink -- ':(exclude)shared/nm-utils/c-list.h') \
              -i
      b6efac9e
  6. 27 Nov, 2017 1 commit
    • Thomas Haller's avatar
      core: fix race of blocking autoconnect for no-secrets when a new secret-agent registers · e2c8ef45
      Thomas Haller authored
      When activation of the connection fails with no-secrets, we block
      autoconnect due to that. However, NMPolicy also unblocks such
      autoconnect, whenever a new secret-agent registers. The reason
      is obviously, that the new secret-agent might be able to provide
      the previously missing secrets.
      
      However, there is a race between
        - making the secret request, failing activation and blocking autoconnect
        - new secret-agent registers
      
      If the secret-agent registers after making the request, but before we
      block autoconnect, then autoconnect stays blocked.
      
        [1511468634.5759] device (wlp4s0): state change: config -> need-auth (reason 'none', sys-iface-state: 'managed')
        [1511468634.5772] device (wlp4s0): No agents were available for this request.
        [1511468638.4082] agent-manager: req[0x55ea7e58a5d0, :1.32/org.kde.plasma.networkmanagement/1000]: agent registered
        [1511468638.4082] policy: re-enabling autoconnect for all connections with failed secrets
        [1511468664.6280] device (wlp4s0): state change: need-auth -> failed (reason 'no-secrets', sys-iface-state: 'managed')
        [1511468664.6287] policy: connection 'tuxmobil' now blocked from autoconnect due to no secrets
      
      Note the long timing between making the secret request and the
      activation failure. This race already existed before, but now with
      WPS push-button method enabled by default, the duraction of the
      activation is much longer and the race is easy to hit.
      
      https://bugzilla.gnome.org/show_bug.cgi?id=790571
      e2c8ef45
  7. 24 Nov, 2017 2 commits
  8. 17 Nov, 2017 1 commit
  9. 11 May, 2017 1 commit
  10. 03 Apr, 2017 1 commit
  11. 24 Mar, 2017 1 commit
  12. 23 Nov, 2016 1 commit
  13. 03 Oct, 2016 2 commits
  14. 08 Jul, 2016 1 commit
  15. 19 Feb, 2016 1 commit
    • Thomas Haller's avatar
      all: cleanup includes and let "nm-default.h" include "config.h" · 8bace23b
      Thomas Haller authored
      - All internal source files (except "examples", which are not internal)
        should include "config.h" first. As also all internal source
        files should include "nm-default.h", let "config.h" be included
        by "nm-default.h" and include "nm-default.h" as first in every
        source file.
        We already wanted to include "nm-default.h" before other headers
        because it might contains some fixes (like "nm-glib.h" compatibility)
        that is required first.
      
      - After including "nm-default.h", we optinally allow for including the
        corresponding header file for the source file at hand. The idea
        is to ensure that each header file is self contained.
      
      - Don't include "config.h" or "nm-default.h" in any header file
        (except "nm-sd-adapt.h"). Public headers anyway must not include
        these headers, and internal headers are never included after
        "nm-default.h", as of the first previous point.
      
      - Include all internal headers with quotes instead of angle brackets.
        In practice it doesn't matter, because in our public headers we must
        include other headers with angle brackets. As we use our public
        headers also to compile our interal source files, effectively the
        result must be the same. Still do it for consistency.
      
      - Except for <config.h> itself. Include it with angle brackets as suggested by
        https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
      8bace23b
  16. 06 Jan, 2016 1 commit
    • Beniamino Galvani's avatar
      core: always use gulong to store signal handler ids · f96abc8b
      Beniamino Galvani authored
      We inconsistently use gulong,guint,int types to store signal handler
      id, but the type returned by g_signal_connect() is a gulong.
      
      This has no practical consequences because a int/guint is enough to
      store the value, however it is better to use a consistent type, also
      because nm_clear_g_signal_handler() accepts a pointer to the signal id
      and thus it must be always called with the same pointer type.
      f96abc8b
  17. 18 Nov, 2015 1 commit
    • Dan Williams's avatar
      exported-object: add support for DBus ObjectManager interface · b023d075
      Dan Williams authored
      NMExportedObject now derives from GDBusObjectSkeleton, which is what
      GDBusObjectManagerServer wants.  The main GDBusConnection and each
      private server connection now gets a new GDBusObjectManagerServer,
      and exported objects are registered with that instead of individually
      exporting each GDBusInterfaceSkeleton.
      
      Previously exported objects were not referenced by the BusManager,
      but instead removed from the exports hash via weak references.  The
      GDBusObjectManagerServer instead references exported objects, which
      can make them live much longer than they did before.
      Co-Authored-By: Thomas Haller's avatarThomas Haller <thaller@redhat.com>
      b023d075
  18. 06 Nov, 2015 1 commit
    • Thomas Haller's avatar
      logging: swap names of logging macros _LOGT() and _LOGt() · 95878673
      Thomas Haller authored
      Previsously, _LOGT() could be disabled at compile time. Thus it
      was different then the other macros _LOGD(), _LOGI(), etc.
      
      OTOH, _LOGt() was the macro that always was compiled in.
      
      Swap the name of the macros. Now the upper-case macros are always
      enabled, while the lower-case macro _LOGt() is enabled depending
      on compile configuration.
      95878673
  19. 25 Sep, 2015 1 commit
  20. 18 Sep, 2015 2 commits
  21. 25 Aug, 2015 7 commits
    • Thomas Haller's avatar
      secret-agent: fix detection of disapearing secret-agent · 9b35d29d
      Thomas Haller authored
      The signal "notify:g-name-owner" is only emitted for well-known
      names, but not for unique connection names (":1.x") such as the secret
      agent's connection. Also, it will not be emited for private connections.
      
      That meant that when the client disconnected without explicitly
      unregistering, the NMSecretAgent was not cleaned up and leaked
      indefinitely.
      As only one instance of secret agent with a certain 'identifier/uid'
      pair can register, this bug also prevented the client from registering
      until restart of NetworkManager.
      
      Fixes: df670681
      9b35d29d
    • Thomas Haller's avatar
      0b3e0215
    • Thomas Haller's avatar
      agent-manager: remove @asked field from request · ea14cd45
      Thomas Haller authored
      This code was unused, because we never enqueued any hashes
      to the @asked list. Note that hashing also might give wrong
      hash collisions, so this was buggy anyway.
      
      Also, note that impl_agent_manager_register_with_capabilities()
      already ensures that duplicate agents are not registered
      in the first place (find_agent_by_identifier_and_uid()).
      ea14cd45
    • Thomas Haller's avatar
      secret-agent: don't assert against existing getpwuid() entry · e5c59d1f
      Thomas Haller authored
      There is a race and there is no guarantee that getpwuid() can lookup a
      uid that (previously) existed. Just accept %NULL as @owner_username.
      e5c59d1f
    • Thomas Haller's avatar
      secret-agent: rework handling of asynchronous request and cancelling · 92dda647
      Thomas Haller authored
      Refactor the handling of the asynchronous requests so that now
      NMSecretAgent has the following properties:
      
      - The callback will *always* be invoked exactly once (sans crashes).
        Even if you cancel the call or if you dispose NMSecretAgent with
        pending calls. That allows the caller to rely on being called back
        and possibly cleanup the user-data.
      
      - Callbacks are always invoked asynchronously with respect to their
        start-call.
      
      - You can cancel all 3 types of operations, not only the 'GetSecrets'
        call. Note that this will still not cancel the calls 'DeleteSecrets'
        and 'SaveSecrets' on a D-Bus level.
        When cancelling, the callback will be invoked synchronously with
        respect to the cancel call, with an GError indicating the cancellation
        (G_IO_ERROR_CANCELLED).
      
      - During dispose, the callback is also invoked synchronously, with
        some other error reason.
      
      This also fixes a crash where handling of the asynchronous data was
      messed up and the priv->requests hash would end up to containing dangling
      pointers.
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1253407
      92dda647
    • Thomas Haller's avatar
      88e485bc
    • Thomas Haller's avatar
      secret-agent: fix leak of @dbus_owner · 8ed98a38
      Thomas Haller authored
      The @dbus_owner field was only cleaned up when the
      proxy disconnected and leaked otherwise.
      
      Also, don't clear @dbus_owner together with the proxy.
      Otherwise, get_description() might yield different results
      after the proxy got cleared. That can lead to problems because
      NMAgentManager tracks the secrets agents by their "dbus-owner" --
      IOW, NMAgentManager uses the "dbus-owner" as identifer for the
      secret agent. Thus it must not change.
      
      Fixes: 2a2fd121
      8ed98a38
  22. 10 Aug, 2015 1 commit
  23. 05 Aug, 2015 1 commit
  24. 30 Jul, 2015 1 commit
  25. 24 Jul, 2015 3 commits
    • Dan Winship's avatar
      core: rename NMDBusManager to NMBusManager · 02370be7
      Dan Winship authored
      Our gdbus generated types use the same names as their corresponding
      "real" types, but with "NM" changed to "NMDBus".
      
      Unfortunately, that means that introspection/nmdbus-manager.c (the
      generated type for src/nm-manager.c) uses the same type name as the
      entirely unrelated src/nm-dbus-manager.c.
      
      Fix this by removing the "d" from src/nm-dbus-manager.c. (We could
      rename the generated type instead, but then it becomes inconsistent
      with all the other generated types, and we're already using it as
      "NMDBusManager" in libnm/nm-manager.c.)
      02370be7
    • Dan Winship's avatar
      settings: rework NMSecretAgent disconnection detection · 2a2fd121
      Dan Winship authored
      Have NMSecretAgent emit "disconnected" when it detects that it has
      been disconnected, rather than having both the agent and the agent
      manager monitor it separately.
      2a2fd121
    • Dan Winship's avatar
      all: rename nm-glib-compat.h to nm-glib.h, use everywhere · 3452ee2a
      Dan Winship authored
      Rather than randomly including one or more of <glib.h>,
      <glib-object.h>, and <gio/gio.h> everywhere (and forgetting to include
      "nm-glib-compat.h" most of the time), rename nm-glib-compat.h to
      nm-glib.h, include <gio/gio.h> from there, and then change all .c
      files in NM to include "nm-glib.h" rather than including the glib
      headers directly.
      
      (Public headers files still have to include the real glib headers,
      since nm-glib.h isn't installed...)
      
      Also, remove glib includes from header files that are already
      including a base object header file (which must itself already include
      the glib headers).
      3452ee2a
  26. 11 Dec, 2014 1 commit
  27. 13 Nov, 2014 1 commit
    • Dan Winship's avatar
      all: consistently include config.h · 3bfb163a
      Dan Winship authored
      config.h should be included from every .c file, and it should be
      included before any other include. Fix that.
      
      (As a side effect of how I did this, this also changes us to
      consistently use "config.h" rather than <config.h>. To the extent that
      it matters [which is not much], quotes are more correct anyway, since
      we're talking about a file in our own build tree, not a system
      include.)
      3bfb163a