1. 07 May, 2019 22 commits
    • Thomas Haller's avatar
      libnm: rename "memory" parameter of fq_codel QDisc to "memory_limit" · 666d5880
      Thomas Haller authored
      Kernel calls the netlink attribute TCA_FQ_CODEL_MEMORY_LIMIT. Likewise,
      iproute2 calls this "memory_limit".
      Rename because TC parameters are inherrently tied to the kernel
      implementation and we should use the familiar name.
    • Thomas Haller's avatar
      platform: fix handling of default value for TCA_FQ_CODEL_CE_THRESHOLD · 973db2d4
      Thomas Haller authored
      iproute2 uses the special value ~0u to indicate not to set
      TCA_FQ_CODEL_CE_THRESHOLD in RTM_NEWQDISC. When not explicitly
      setting the value, kernel treats the threshold as disabled.
      However note that 0xFFFFFFFFu is not an invalid threshold (as far as
      kernel is concerned). Thus, we should not use that as value to indicate
      that the value is unset. Note that iproute2 uses the special value ~0u
      only internally thereby making it impossible to set the threshold to
      0xFFFFFFFFu). But kernel does not have this limitation.
      Maybe the cleanest way would be to add another field to NMPlatformQDisc:
          guint32 ce_threshold;
          bool ce_threshold_set:1;
      that indicates whether the threshold is enable or not.
      But note that kernel does:
          static void codel_params_init(struct codel_params *params)
                  params->ce_threshold = CODEL_DISABLED_THRESHOLD;
          static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
                                     struct netlink_ext_ack *extack)
                  if (tb[TCA_FQ_CODEL_CE_THRESHOLD]) {
                          u64 val = nla_get_u32(tb[TCA_FQ_CODEL_CE_THRESHOLD]);
                          q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT;
          static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
                  if (q->cparams.ce_threshold != CODEL_DISABLED_THRESHOLD &&
                      nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD,
                          goto nla_put_failure;
      This means, kernel internally uses the special value 0x83126E97u to indicate
      that the threshold is disabled (WTF). That is because
        (((guint64) 0x83126E97u) * NSEC_PER_USEC) >> CODEL_SHIFT == CODEL_DISABLED_THRESHOLD
      So in kernel API this value is reserved (and has a special meaning
      to indicate that the threshold is disabled). So, instead of adding a
      ce_threshold_set flag, use the same value that kernel anyway uses.
    • Thomas Haller's avatar
      platform: fix handling of fq_codel's memory limit default value · 46a90438
      Thomas Haller authored
      The memory-limit is an unsigned integer. It is ugly (if not wrong) to compare unsigned
      values with "-1". When comparing with the default value we must also use an u32 type.
      Note that like iproute2 we treat NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
      to indicate to not set TCA_FQ_CODEL_MEMORY_LIMIT in RTM_NEWQDISC. This
      special value is entirely internal to NetworkManager (or iproute2) and
      kernel will then choose a default memory limit (of 32MB). So setting
      NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET means to leave it to kernel to
      choose a value (which then chooses 32MB).
      See kernel's net/sched/sch_fq_codel.c:
          static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
                                   struct netlink_ext_ack *extack)
                  q->memory_limit = 32 << 20; /* 32 MBytes */
          static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
                                     struct netlink_ext_ack *extack)
                  if (tb[TCA_FQ_CODEL_MEMORY_LIMIT])
                          q->memory_limit = min(1U << 31, nla_get_u32(tb[TCA_FQ_CODEL_MEMORY_LIMIT]));
      Note that not having zero as default value is problematic. In fields like
      "NMPlatformIP4Route.table_coerced" and "NMPlatformRoutingRule.suppress_prefixlen_inverse"
      we avoid this problem by storing a coerced value in the structure so that zero is still
      the default. We don't do that here for memory-limit, so the caller must always explicitly
      set the value.
    • Thomas Haller's avatar
      platform: fix nm_platform_qdisc_to_string() · b658e3da
      Thomas Haller authored
      When using nm_utils_strbuf_*() API, the buffer gets always moved to the current
      end. We must thus remember and return the original start of the buffer.
    • Thomas Haller's avatar
      platform: use u32 netlink type for TCA_FQ_CODEL_ECN · a1099a1f
      Thomas Haller authored
      In practice, there is no difference when representing 0 or 1 as signed/unsigned 32
      bit integer. But still use the correct type that also kernel uses.
      Also, the implicit conversation from uint32 to bool was correct already.
      Still, explicitly convert the uint32 value to boolean in _new_from_nl_qdisc().
      It's no change in behavior.
    • Thomas Haller's avatar
      platform: use NM_CMP_FIELD_UNSAFE() for comparing bitfield in nm_platform_qdisc_cmp() · 47d8bee1
      Thomas Haller authored
      "NM_CMP_FIELD (a, b, fq_codel.ecn == TRUE)" is quite a hack as it relies on
      the implementation of the macro in a particular way. The problem is, that
      NM_CMP_FIELD() uses typeof() which cannot be used with bitfields. So, the
      nicer solution is to use NM_CMP_FIELD_UNSAFE() which exists exactly for bitfields
      (it's "unsafe", because it evaluates arguments more than once as it avoids
      the temporary variable with typeof()).
      Same with nm_hash_update_vals() which uses typeof() to avoid evaluating
      arguments more than once. But that again does not work with bitfields.
      The "proper" way is to use NM_HASH_COMBINE_BOOLS().
    • Thomas Haller's avatar
      device: fix type of loop variable in tc_commit() · 438855e9
      Thomas Haller authored
      nqdiscs and ntfilters are unsigned integers. The loop variable must agree in
      range and signedness.
    • Thomas Haller's avatar
      libnm: use macro and designated initializers for NMVariantAttributeSpec · 86dc50d4
      Thomas Haller authored
      I think initializing structs should (almost) be always done with designated
      initializers, because otherwise it's easy to get the order wrong. The
      problem is that otherwise the order of fields gets additional meaning
      not only for the memory layout, but also for the code that initialize
      the structs.
      Add a macro NM_VARIANT_ATTRIBUTE_SPEC_DEFINE() that replaces the other
      (duplicate) macros. This macro also gets it right to mark the struct as
    • Thomas Haller's avatar
      libnm: mark NMVariantAttributeSpec pointers as const · 4e3955e6
      Thomas Haller authored
      This actually allows the compiler/linker to mark the memory as read-only and any
      modification will cause a segmentation fault.
      I would also think that it allows the compiler to put the structure directly
      beside the outer constant array (in which this pointer is embedded). That is good
    • Thomas Haller's avatar
      libnm: cleanup _nm_utils_parse_tc_handle() · cc9f0716
      Thomas Haller authored
      - g_ascii_strtoll() accepts leading spaces, but it leaves
        the end pointer at the first space after the digit. That means,
        we accepted "1: 0" but not "1 :0". We should either consistently
        accept spaces around the digits/colon or reject it.
      - g_ascii_strtoll() accepts "\v" as a space (just like `man 3 isspace`
        comments that "\v" is a space in C and POSIX locale.
        For some reasons (unknown to me) g_ascii_isspace() does not treat
        "\v" as space. And neither does NM_ASCII_SPACES and
        We should be consistent about what we consider spaces and what not.
        It's already odd to accept '\n' as spaces here, but well, lets do
        it for the sake of consistency (so that it matches with our
        understanding of ASCII spaces, albeit not POSIX's).
      - don't use bogus error domains in "g_set_error (error, 1, 0, ..."
        That is a bug and we have NM_UTILS_ERROR exactly for error instances
        with unspecified domain and code.
      - as before, accept a trailing ":" with omitted minor number.
      - reject all unexpected characters. strtoll() accepts '+' / '-'
        and a "0x" prefix of the numbers (and leading POSIX spaces). Be
        strict here and only accepts NM_ASCII_SPACES, ':', and hexdigits.
        In particular, don't accept the "0x" prefix.
      This parsing would be significantly simpler to implement, if we could
      just strdup() the string, split the string at the colon delimiter and
      use _nm_utils_ascii_str_to_int64() which gets leading/trailing spaces
      right. But let's save the "overhead" of an additional alloc.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Lubomir Rintel's avatar
      modem/broadband: set the gsm.device-id in complete_connection() · 143f518c
      Lubomir Rintel authored
      This is the preferred way of associating the connection with a
      particualr modem.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      settings: cache keyfile databases for "timestamps" and "seen-bssids" · 8a78493d
      Thomas Haller authored
      Only read the keyfile databases once and cache them for the remainder of
      the program.
      - this avoids the overhead of opening the file over and over again.
      - it also avoids the data changing without us expecting it. The state
        files are internal and we don't support changing it outside of
        NetworkManager. So in the base case we read the same data over
        and over. In the worst case, we read different data but are not
        interested in handling the changes.
      - only write the file when the content changes or before exiting
      - better log what is happening.
      - our state files tend to grow as we don't garbage collect old entries.
        Keeping this all in memory might be problematic. However, the right
        solution for this is that we come up with some form of garbage
        collection so that the state files are reaonsably small to begin with.
    • Thomas Haller's avatar
      shared: add NMKeyFileDB API · b0693863
      Thomas Haller authored
      It will be used for "/var/lib/NetworkManager/seen-bssids" and
      "/var/lib/NetworkManager/timestamps" which currently is implemented
      in NMSettingConnection.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      core: use NM_SETTINGS_GET for singlton instead of nm_settings_get() · 4aba7d46
      Thomas Haller authored
      We have it, so use it. Also, we use a similar macro for other singletons.
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      platform/ethtool,mii: retry ioctl when interface name was renamed for ehttool/mii · 85632256
      Thomas Haller authored
      ethtool/mii API is based on the ifname. As an interface can be renamed,
      this API is inherently racy. We would prefer to use the ifindex instead.
      The ifindex of a device cannot change (altough it can repeat, which opens a
      different race *sigh*).
      Anyway, we were already trying to minimize the race be resolving the
      name from ifindex immediately before the call to ethtool/mii.
      Do better than that. Now resolve the name before and after the call. If
      the name changed in the meantime, we have an indication that a race
      might have happend (but we cannot be sure).
      Note that this can not catch every possible kind of rename race. If you are very
      unlucky a swapping of names cannot be detected.
      For getters this is relatively straight forward. Just retry when we
      have an indication to fall victim to a race (up to a few times). Yes, we
      still cannot be 100% sure, but this should be very reliable in practice.
      For setters (that modify the device) we also retry. We do so under the
      assumption that setting the same options multiple times has no bad effect.
      Note that for setters the race of swapping interface names is particularly
      bad. If we hit a very unlucky race condition, we might set the setting on
      the wrong interface and there is nothing we can do about it. The retry only
      ensures that eventually we will set it on the right interface.
      Note that this involves one more if_indextoname() call for each operation (in
      the common case when there is no renaming race). In cases where we make
      multiple ioctl calls, we cache and reuse the information though. So, for such
      calls the overhead is even smaller.
    • Thomas Haller's avatar
  2. 06 May, 2019 9 commits
  3. 04 May, 2019 1 commit
  4. 03 May, 2019 4 commits
  5. 02 May, 2019 1 commit
    • Thomas Haller's avatar
      platform/tests: workaround routing-rules test failure due to suppress_prefixlen on older kernels · d5a2b709
      Thomas Haller authored
      On Ubuntu 14.04 kernel (4.4.0-146-generic, x86_64) this easily causes
      test failures:
          make -j 8 src/platform/tests/test-route-linux \
          && while true; do \
              NMTST_SEED_RANDOM= ./tools/run-nm-test.sh src/platform/tests/test-route-linux -p /route/rule \
              || break; \
          nmtst: initialize nmtst_get_rand() with NMTST_SEED_RAND=22892021
          /route/rule/2: >>> failing...
          >>> no fuzzy match between: [routing-rule,0x205ab30,1,+alive,+visible; [6] 0: from all suppress_prefixlen 8 none]
          >>>                    and: [routing-rule,0x205c0c0,1,+alive,+visible; [6] 0: from all suppress_prefixlen -1579099242 none]
          test:ERROR:src/platform/tests/test-route.c:1695:test_rule: code should not be reached
  6. 01 May, 2019 3 commits