1. 27 May, 2019 16 commits
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      build: don't link dispatcher with generated nmdbus-dispatcher bindings · f8096448
      Thomas Haller authored
      We don't need it anymore.
      
      Still, for tests let gdbus-codegen run and generate the sources and
      compile them. We want to keep "dispatcher/nm-dispatcher.xml" and ensure
      that it is still valid.
      f8096448
    • Thomas Haller's avatar
      cdaedeca
    • Thomas Haller's avatar
      dispatcher: drop GDBusInterfaceSkeleton and generate GDBus bindings · a01e7606
      Thomas Haller authored
      Just hook into GDBusConnection directly. No need for this additional
      layer that provides nothing. It doesn't even provide extra type-safety,
      because you still need to get the arguments of the signal handler right.
      That that point, it's as hard as getting the format string for
      g_variant_get() right.
      
      It still adds lines of code, because the "Action" method has such a
      large argument list.
      a01e7606
    • Thomas Haller's avatar
      dispatcher: drop Handler GObject · 484194fa
      Thomas Haller authored
      "nm-dispatcher.c" does something rather simple. It is natural that it
      has a bit of global data to keep track of that it's doing ("gl").
      
      But this does not lend itself to pack the job of dispatcher into an
      object. In fact, the Handler object was little more about packaging
      the GDBus interface skeleton and a bit of state.
      
      Global variables are often problematic because they makes unit testing hard.
      But first of all, we have no test for this (we should). But it's not said
      that you need an "object" to make testing easier. If we want to make
      individual bits easier testable, we can just as well pass all required
      parameters explicitly instead of accessing global variables. Since we
      package global variables neatly in "gl", this is very simple to
      refactor. Also, global variables can make code harder to understand. But
      the problem is the amount of state that is accessible. This is not
      alleviated by packaging the state in a Handler object.
      
      As there is always only one handler instance, this provides very little
      benefit.
      
      I will drop the GDBus interface skeleton soon. So this Handler object
      will have even less purpose. Drop it.
      484194fa
    • Thomas Haller's avatar
      dispatcher: keep the GDBusConnection instance in the global variable · cdea5ca7
      Thomas Haller authored
      It's anyway a singleton that is still referenced by other components.
      So unrefing it in the mainloop does not actually release any memory.
      
      However, the GDBusConnection singleton is fundamental for the run of
      the program. Keep it accessible in the global variables.
      
      Note that soon I will drop the GDBusInterfaceSkeleton and only operate
      on the GDBusConnection. Then it makes more sense to keep it around.
      
      Note that usually we want to keep the amount of global state small.
      But this connection is anyway a singleton (that we already implicitly
      use). So, it doesn't change the amount of global state nor does it really
      have much state (we either have a reference to the singleton or we don't).
      cdea5ca7
    • Thomas Haller's avatar
      dispatcher: parse command line arguments in a seprate function · 3a3c807a
      Thomas Haller authored
      Split command line parsing out of the main() function. For one, it's
      an self-contained step, so we can make main() simpler.
      
      Also, we don't need the GOptionEntry on the stack of the main() function
      for the remainder of the program.
      3a3c807a
    • Thomas Haller's avatar
      dispatcher: don't just exit() but always shutdown before exiting · 866189a0
      Thomas Haller authored
      It's ugly to uncoordinated just call exit(). We should quit the mainloop
      and clean up everything we had going.
      
      Note that since Handler has no dispose() function, we also need to hack
      a g_signal_handlers_disconnect_by_func(). This will change soon.
      866189a0
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      bccf7543
    • Thomas Haller's avatar
      dispatcher: use logging macros instead of g_log() directly · 23286b2a
      Thomas Haller authored
      Also cleanup the logging macros.
      23286b2a
    • Thomas Haller's avatar
      dispatcher: silence message about non-existing dispatcher directory · b4d678b3
      Thomas Haller authored
      Silence messages like
      
        find-scripts: Failed to open dispatcher directory '/usr/lib/NetworkManager/dispatcher.d': Error opening directory “/usr/lib/NetworkManager/dispatcher.d”: No such file or directory
      
        find-scripts: Failed to open dispatcher directory '/usr/lib/NetworkManager/dispatcher.d/pre-up.d': Error opening directory “/usr/lib/NetworkManager/dispatcher.d/pre-up.d”: No such file or directory
      b4d678b3
    • Thomas Haller's avatar
      dispatcher: log messages about loading scripts per-request · 8518a873
      Thomas Haller authored
      The messages about loading the scripts are releated to a particular
      request. Hence, they should be logged with that context.
      
      Also, we should avoid using g_log() directly. Use our logging macros
      instead.
      8518a873
    • Thomas Haller's avatar
      dispatcher: don't print debug messages of dispatcher in regular mode · 58704a37
      Thomas Haller authored
      Previously, we would log several messages with level "debug" / g_info().
      
        _LOG_R_D (request, "start running ordered scripts...");
        _LOG_R_D (request, "new request (%u scripts)", request->scripts->len);
        _LOG_R_D (request, "completed: no scripts");
      
      Note that this effectively logs a message for every event. I think that
      is to verbose and not suitable for regular logging.
      
      Only enable these messages if debug logging is enabled. As such, these debug
      level messsages now are enabled together with the trace level messages.
      58704a37
    • Thomas Haller's avatar
      dispatcher/trivial: rename logging levels · dd72696a
      Thomas Haller authored
      What we currently print as "info" level is too verbose for regular
      operation. It prints two messages for every dispatcher event. That's
      already for debugging.
      
      Next that will be downgraded, so rename "debug" to "trace" and "info" to
      "debug".
      
      There is only renaming, no change in behavior.
      dd72696a
  2. 29 Apr, 2019 1 commit
  3. 18 Apr, 2019 4 commits
    • Thomas Haller's avatar
      build/meson: rename "nm_core_dep" to "libnm_core_dep" · e7836cd1
      Thomas Haller authored
      The library is called "libnm_core". So the dependency should be called
      "libnm_core_dep", like in all other cases.
      
      (cherry picked from commit c27ad37c)
      e7836cd1
    • Thomas Haller's avatar
      shared: build helper "libnm-libnm-core-{intern|aux}.la" library for libnm-core · 284ac92e
      Thomas Haller authored
      "libnm-core" implements common functionality for "NetworkManager" and
      "libnm".
      
      Note that clients like "nmcli" cannot access the internal API provided
      by "libnm-core". So, if nmcli wants to do something that is also done by
      "libnm-core", , "libnm", or "NetworkManager", the code would have to be
      duplicated.
      
      Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
      Note that:
      
        0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
           On the other hand, "libnm-libnm-core-aux.la" is not used by
           libnm-core, but provides utilities on top of it.
      
        1) they both extend "libnm-core" with utlities that are not public
           API of libnm itself. Maybe part of the code should one day become
           public API of libnm. On the other hand, this is code for which
           we may not want to commit to a stable interface or which we
           don't want to provide as part of the API.
      
        2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
           and thus directly available to "libnm" and "NetworkManager".
           On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
           and "NetworkManager".
           Both libraries may be statically linked by libnm clients (like
           nmcli).
      
        3) it must only use glib, libnm-glib-aux.la, and the public API
           of libnm-core.
           This is important: it must not use "libnm-core/nm-core-internal.h"
           nor "libnm-core/nm-utils-private.h" so the static library is usable
           by nmcli which couldn't access these.
      
      Note that "shared/nm-meta-setting.c" is an entirely different case,
      because it behaves differently depending on whether linking against
      "libnm-core" or the client programs. As such, this file must be compiled
      twice.
      
      (cherry picked from commit af07ed01)
      284ac92e
    • Thomas Haller's avatar
      build/meson: rename "nm_core_dep" to "libnm_core_dep" · c27ad37c
      Thomas Haller authored
      The library is called "libnm_core". So the dependency should be called
      "libnm_core_dep", like in all other cases.
      c27ad37c
    • Thomas Haller's avatar
      shared: build helper "libnm-libnm-core-{intern|aux}.la" library for libnm-core · af07ed01
      Thomas Haller authored
      "libnm-core" implements common functionality for "NetworkManager" and
      "libnm".
      
      Note that clients like "nmcli" cannot access the internal API provided
      by "libnm-core". So, if nmcli wants to do something that is also done by
      "libnm-core", , "libnm", or "NetworkManager", the code would have to be
      duplicated.
      
      Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
      Note that:
      
        0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
           On the other hand, "libnm-libnm-core-aux.la" is not used by
           libnm-core, but provides utilities on top of it.
      
        1) they both extend "libnm-core" with utlities that are not public
           API of libnm itself. Maybe part of the code should one day become
           public API of libnm. On the other hand, this is code for which
           we may not want to commit to a stable interface or which we
           don't want to provide as part of the API.
      
        2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
           and thus directly available to "libnm" and "NetworkManager".
           On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
           and "NetworkManager".
           Both libraries may be statically linked by libnm clients (like
           nmcli).
      
        3) it must only use glib, libnm-glib-aux.la, and the public API
           of libnm-core.
           This is important: it must not use "libnm-core/nm-core-internal.h"
           nor "libnm-core/nm-utils-private.h" so the static library is usable
           by nmcli which couldn't access these.
      
      Note that "shared/nm-meta-setting.c" is an entirely different case,
      because it behaves differently depending on whether linking against
      "libnm-core" or the client programs. As such, this file must be compiled
      twice.
      af07ed01
  4. 12 Apr, 2019 1 commit
    • Thomas Haller's avatar
      dispatcher/tests: cleanup tests · 0d3bf972
      Thomas Haller authored
      - use cleanup macros everywhere.
      
      - In particular use nm_auto_clear_variant_builder to free the
        GVariantBuilder in the error cases. Note that the error cases
        anyway are asserted against, so during a normal test run there
        was no leak. But we should not write software like that.
      
      - use nm_utils_strsplit_set_with_empty() instead of g_strsplit_set().
        We should use our variant also in unit-tests, because that way the
        function gets more test coverage. And it likely performs better
        anyway.
      0d3bf972
  5. 12 Feb, 2019 1 commit
  6. 06 Feb, 2019 1 commit
    • Thomas Haller's avatar
      all: don't use "static inline" in source files · d25ed082
      Thomas Haller authored
      For static functions inside a module, the compiler determines on its own
      whether to inline the function.
      
      Also, "inline" was used at some places that don't immediatly look like
      candidates for inlining. It was most likely a copy&paste error.
      d25ed082
  7. 20 Dec, 2018 1 commit
  8. 17 Jul, 2018 1 commit
    • Thomas Haller's avatar
      build: create "config-extra.h" header instead of passing directory variables via CFLAGS · a75ab799
      Thomas Haller authored
      1) the command line gets shorter. I frequently run `make V=1` to see
         the command line arguments for the compiler, and there is a lot
         of noise.
      
      2) define each of these variables at one place. This makes it easy
         to verify that for all compilation units, a particular
         define has the same value. Previously that was not obvious or
         even not the case (see commit e5d1a713
         and commit d63cf1ef).
         The point is to avoid redundancy.
      
      3) not all compilation units need all defines. In fact, most modules
         would only need a few of these defines. We aimed to pass the necessary
         minium of defines to each compilation unit, but that was non-obvious
         to get right and often we set a define that wasn't used. See for example
         "src_settings_plugins_ibft_cppflags" which needlessly had "-DSYSCONFDIR".
         This question is now entirely avoided by just defining all variables in
         a header. We don't care to find the minimum, because every component
         gets anyway all defines from the header.
      
      4) this also avoids the situation, where a module that previously did
         not use a particular define gets modified to require it. Previously,
         that would have required to identify the missing define, and add
         it to the CFLAGS of the complation unit. Since every compilation
         now includes "config-extra.h", all defines are available everywhere.
      
      5) the fact that each define is now available in all compilation units
         could be perceived as a downside. But it isn't, because these defines
         should have a unique name and one specific value. Defining the same
         name with different values, or refer to the same value by different
         names is a bug, not a desirable feature. Since these defines should
         be unique accross the entire tree, there is no problem in providing
         them to every compilation unit.
      
      6) the reason why we generate "config-extra.h" this way, instead of using
         AC_DEFINE() in configure.ac, is due to the particular handling of
         autoconf for directory variables. See [1].
         With meson, it would be trivial to put them into "config.h.meson".
         While that is not easy with autoconf, the "config-extra.h" workaround
         seems still preferable to me.
      
      [1] https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Installation-Directory-Variables.html
      a75ab799
  9. 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
  10. 18 Jun, 2018 4 commits
    • Thomas Haller's avatar
      dispatcher/tests: fix test after adding NM_DISPATCHER_ACTION · ccf1bc7a
      Thomas Haller authored
      Also, fix existing tests regarding connectivity action.
      
      Fixes: ce961904
      (cherry picked from commit 6dec8ea9)
      ccf1bc7a
    • Thomas Haller's avatar
      dispatcher/tests: fix test after adding NM_DISPATCHER_ACTION · 6dec8ea9
      Thomas Haller authored
      Also, fix existing tests regarding connectivity action.
      
      Fixes: ce961904
      6dec8ea9
    • Thomas Haller's avatar
      dispatcher: add NM_DISPATCHER_ACTION environment variable · 2800232b
      Thomas Haller authored
      Previously, the action was only passed as the first command line
      argument to the dispatcher scripts. Now, also set it via the
      "$NM_DISPATCHER_ACTION" environment variable.
      
      The main purpose is to have a particular, nm-dispatcher specific
      variable that is always set inside the dispatcher scripts.
      For example, imagine you have a script that can be either called by
      dispatcher or some other means (manually, or spawned via
      /etc/NetworkManager/dispatcher.d/11-dhclient).  Then it might make
      sense to differenciate from inside the script whether you are called
      by nm-dispatcher. But previously, there was no specific environment
      variable that was always set inside the dispatcher event. For example,
      with the "hostname" action there are no other environment variables.
      
      Now (with version 1.12), you can check for `test -n "$NM_DISPATCHER_ACTION"`.
      
      (cherry picked from commit ce961904)
      2800232b
    • Thomas Haller's avatar
      dispatcher: add NM_DISPATCHER_ACTION environment variable · ce961904
      Thomas Haller authored
      Previously, the action was only passed as the first command line
      argument to the dispatcher scripts. Now, also set it via the
      "$NM_DISPATCHER_ACTION" environment variable.
      
      The main purpose is to have a particular, nm-dispatcher specific
      variable that is always set inside the dispatcher scripts.
      For example, imagine you have a script that can be either called by
      dispatcher or some other means (manually, or spawned via
      /etc/NetworkManager/dispatcher.d/11-dhclient).  Then it might make
      sense to differenciate from inside the script whether you are called
      by nm-dispatcher. But previously, there was no specific environment
      variable that was always set inside the dispatcher event. For example,
      with the "hostname" action there are no other environment variables.
      
      Now (with version 1.12), you can check for `test -n "$NM_DISPATCHER_ACTION"`.
      ce961904
  11. 31 May, 2018 1 commit
    • Thomas Haller's avatar
      build: use default NM_BUILD_* defines for tests · b7426e91
      Thomas Haller authored
      Use two common defines NM_BUILD_SRCDIR and NM_BUILD_BUILDDIR
      for specifying the location of srcdir and builddir.
      
      Note that this is only relevant for tests, as they expect
      a certain layout of the directories, to find files that concern
      them.
      b7426e91
  12. 24 May, 2018 1 commit
  13. 16 May, 2018 2 commits
  14. 10 May, 2018 1 commit
    • Lubomir Rintel's avatar
      all: use the elvis operator wherever possible · e69d3869
      Lubomir Rintel authored
      Coccinelle:
      
        @@
        expression a, b;
        @@
        -a ? a : b
        +a ?: b
      
      Applied with:
      
        spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .
      
      With some manual adjustments on spots that Cocci didn't catch for
      reasons unknown.
      
      Thanks to the marvelous effort of the GNU compiler developer we can now
      spare a couple of bits that could be used for more important things,
      like this commit message. Standards commitees yet have to catch up.
      e69d3869
  15. 30 Apr, 2018 1 commit
  16. 12 Apr, 2018 2 commits
  17. 07 Feb, 2018 1 commit