1. 18 Apr, 2019 33 commits
    • Thomas Haller's avatar
      shared: move udev helper to separate directory "shared/nm-udev-aux" · 2973d682
      Thomas Haller authored
      We built (among others) two libraries from the sources in "shared/nm-utils":
      "libnm-utils-base.la" and "libnm-utils-udev.la".
      
      It's confusing. Instead use directories so there is a direct
      correspondence between these internal libraries and the source files.
      2973d682
    • Thomas Haller's avatar
      shared: split C-only helper "shared/nm-std-aux" utils out of "shared/nm-utils" · b434b9ec
      Thomas Haller authored
      "shared/nm-utils" contains general purpose utility functions that only
      depend on glib (and extend glib with some helper functions).
      
      We will also add code that does not use glib, hence it would be good
      if the part of "shared/nm-utils" that does not depend on glib, could be
      used by these future projects.
      
      Also, we use the term "utils" everywhere. While that covers the purpose
      and content well, having everything called "nm-something-utils" is not
      great. Instead, call this "nm-std-aux", inspired by "c-util/c-stdaux".
      b434b9ec
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      cli: use "escaped-tokens" style for splitting "vlan.xgress-priority-map" list · 7f01da91
      Thomas Haller authored
      There should be little difference here, because the priority list is
      (and was) never serialized with special characters like backslashes or
      delimiters that require escaping.
      
      Likewise, no working code actually tried to set such characters.
      
      Still, drop the plain VALUE_STRSPLIT_MODE_STRIPPED and use
      VALUE_STRSPLIT_MODE_ESCAPED_TOKENS_WITH_SPACES instead. We should have
      a small set of modes that we use for splitting strings.
      7f01da91
    • Thomas Haller's avatar
      cli: cleanup _get_fcn_vlan_xgress_priority_map() · bbfd3668
      Thomas Haller authored
      - merge the pointless helper function vlan_priorities_to_string()
        into the only caller _get_fcn_vlan_xgress_priority_map().
      
      - minor cleanups, like setting out-is-default if num==0, not
        based on whether we have a non-empty string. There is not difference
        in practice, because nm_setting_vlan_get_priority() never fails.
        Hence they are identical. But nm_setting_vlan_get_priority() has
        an API that allows it to fail, so we should declare the default
        depending on the number of vlan priorities.
      
      - don't allocate the temporary GString instance if we won't need it.
      
      - only append the delimiter if needed, and not truncate it afterwards.
        It might have even worse performance this way, but it feels more
        correct to me.
      
      - also cache the result of nm_setting_vlan_get_num_priorities().
        NMSettingVlan's implementation is horrible and uses a GSList to
        track the list of priorities. This makes it relatively expensive
        to call get-num-priorities repeatedly (and pointless).
      bbfd3668
    • Thomas Haller's avatar
      cli: unify set of characters to tokenize list properties · 7a92fb6b
      Thomas Haller authored
      the only change in behaviour is for VALUE_STRSPLIT_MODE_MULTILIST.
      Previously, we would split at " \t,", now we will also split at
      the white space characters "\n\r\f".
      7a92fb6b
    • Thomas Haller's avatar
      shared: remove unused _nm_utils_escape_plain()/_nm_utils_escape_spaces() API · 304eab87
      Thomas Haller authored
      ... and the "unescape" variants.
      
      This is replaced by nm_utils_escaped_tokens_split()
      and nm_utils_escaped_tokens_escape*() API.
      304eab87
    • Thomas Haller's avatar
      ifcfg-rh: use nm_utils_escaped_tokens* for "MATCH_INTERFACE_NAME" · 941f27d3
      Thomas Haller authored
      For one, use NM_ASCII_SPACES as delimiter when reading
      "MATCH_INTERFACE_NAME". Previously, it was only " \t".
      
      I think there is no change in behavior otherwise.
      941f27d3
    • Thomas Haller's avatar
      cli: refactor multilist property handling of "match.interface-names" · 6093f493
      Thomas Haller authored
      We had %VALUE_STRSPLIT_MODE_MULTILIST_WITH_ESCAPE, which was used
      by "match.interface-names". This uses nm_utils_strsplit_set_full()
      with %NM_UTILS_STRSPLIT_SET_FLAGS_ALLOW_ESCAPING and
      _nm_utils_unescape_plain().
      
      We want eventually to use nm_utils_escaped_tokens_split() everywhere.
      
      We already have %VALUE_STRSPLIT_MODE_ESCAPED_TOKENS, which splits the
      list at ',' (and strips whitespaces at the around the delimiter). That
      differs from what %VALUE_STRSPLIT_MODE_MULTILIST_WITH_ESCAPE did, which
      also considered whitespace a delimiter.
      
      So, we need a new mode %VALUE_STRSPLIT_MODE_ESCAPED_TOKENS_WITH_SPACES
      which replaces the previous mode.
      
      Note that the previous implementation did almost the same thing. In
      fact, I cannot imagine examples where they behaved differently, but
      my feeling is that there might be some edge cases where this changes
      behavior.
      6093f493
    • Thomas Haller's avatar
      cli: return early when splitting with %VALUE_STRSPLIT_MODE_STRIPPED · b74d9a0b
      Thomas Haller authored
      The reminder of the function only does (something akin to) g_strstrip().
      As we split the strings are spaces to begin with, there is nothing to
      strip and we can return right away.
      b74d9a0b
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      cli: default splitting list properties with escaped-tokens style · 5a715920
      Thomas Haller authored
      When splitting (and concatenating) list-typed properties,
      we really should use nm_utils_escaped_tokens_split()
      and nm_utils_escaped_tokens_escape*().
      
      Make that the default, and mark all properties to opt-in to the
      legacy behavior.
      5a715920
    • Thomas Haller's avatar
      cli: fix splitting of multilist property in setter · 758bf326
      Thomas Haller authored
      The modes VALUE_STRSPLIT_MODE_OBJLIST* and VALUE_STRSPLIT_MODE_MULTILIST* are
      different. We must use the right mode.
      
      For example, _get_fcn_match_interface_name() concatenates the interface-names
      with space. So, the tokenizer of the setter must also use space as delimiter.
      VALUE_STRSPLIT_MODE_MULTILIST_WITH_ESCAPE does that correctly,
      VALUE_STRSPLIT_MODE_OBJLIST_WITH_ESCAPE does not.
      758bf326
    • Thomas Haller's avatar
      6bef7236
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      platform: compare routing rules according to kernel support for FRA_L3MDEV · b6ff02e7
      Thomas Haller authored
      Also, in nm_platform_routing_rule_cmp() always compare the routing
      table field, also if l3mdev is set. For kernel, we cannot set table and
      l3mdev together, hence such rules don't really exist (or if we try to
      configure it, it will be rejected by kernel). But as far as
      nm_platform_routing_rule_cmp() is concerned, if the table is set,
      always compare it.
      b6ff02e7
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      platform: compare routing rules according to kernel support for FRA_IP_PROTO · 6a6d982c
      Thomas Haller authored
      ... and FRA_SPORT_RANGE and FRA_DPORT_RANGE.
      6a6d982c
    • Thomas Haller's avatar
      platform: compare routing rules according to kernel support for FRA_PROTOCOL · ef4f8ccf
      Thomas Haller authored
      For routes and routing rules, kernel uses a certain (not stictly defined) set
      of attributes to decide whether to routes/rules are identical.
      
      That is a problem, as different kernel versions disagree on whether
      two routes/rules are the same (EEXIST) or not.
      
      Note that when NetworkManager tries to add a rule with protocol set to
      anything but RTPROT_UNSPEC, then kernel will ignore the attribute if it
      doesn't have support for it. Meaning: the added rule will have a
      different protocol setting then intended.
      
      Note that NMPRulesManager will add a rule if it doesn't find it in the
      platform cache so far. That means, when looking into the platform cache
      we must ignore or honor the protocol like kernel does.
      
      This does not only affect FRA_PROTOCOL, but all attributes where kernel
      and NetworkManager disagrees. But the protocol is the most prominent
      one, because the rules tracked by nmp_rules_manager_track_default()
      specify the protocol.
      ef4f8ccf
    • Thomas Haller's avatar
      eba4fd56
    • Thomas Haller's avatar
      1dd1dcb8
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      cd62d439
    • Thomas Haller's avatar
      platform: refactor detecting kernel features · ee269b31
      Thomas Haller authored
      Next we will need to detect more kernel features. First refactor the
      handling of these to require less code changes and be more efficient.
      A plain nm_platform_kernel_support_get() only reqiures to access an
      array in the common case.
      
      The other important change is that the function no longer requires a
      NMPlatform instance. This allows us to check kernel support from
      anywhere. The only thing is that we require kernel support to be
      initialized before calling this function. That means, an NMPlatform
      instance must have detected support before.
      ee269b31
    • Thomas Haller's avatar
      libnm-core/tests: fix "-Werror=logical-not-parentheses" warning in _sock_addr_endpoint() · 1e8c0873
      Thomas Haller authored
          CC       libnm-core/tests/libnm_core_tests_test_general-test-general.o
        In file included from ../shared/nm-default.h:280:0,
                         from ../libnm-core/tests/test-general.c:24:
        ../libnm-core/tests/test-general.c: In function _sock_addr_endpoint:
        ../libnm-core/tests/test-general.c:5911:18: error: logical not is only applied to the left hand side of comparison [-Werror=logical-not-parentheses]
          g_assert (!host == (port == -1));
                          ^
        ../shared/nm-utils/nm-macros-internal.h:1793:7: note: in definition of macro __NM_G_BOOLEAN_EXPR_IMPL
           if (expr) \
               ^
        /usr/include/glib-2.0/glib/gmacros.h:376:43: note: in expansion of macro _G_BOOLEAN_EXPR
         #define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR((expr)), 1))
                                                   ^
        /usr/include/glib-2.0/glib/gtestutils.h:116:49: note: in expansion of macro G_LIKELY
                                                      if G_LIKELY (expr) ; else \
                                                         ^
        ../libnm-core/tests/test-general.c:5911:2: note: in expansion of macro g_assert
          g_assert (!host == (port == -1));
          ^
      
      Fixes: 713e879d ('libnm: add NMSockAddrEndpoint API')
      1e8c0873
    • Beniamino Galvani's avatar
      bridge: merge branch 'bg/bridge-vlan-ranges' · 693252d0
      Beniamino Galvani authored
      In some cases it is convenient to specify ranges of bridge vlans, as
      already supported by iproute2 and natively by kernel.
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1652910
      NetworkManager/NetworkManager!114
      693252d0
    • Thomas Haller's avatar
      libnm: minor refactoring of _nm_utils_bridge_vlan_verify_list() · a358da09
      Thomas Haller authored
      - if there is only one vlan in the list, then we can return success
        early. That is, because one NMBridgeVlan instance is always valid
        due to the way how users must use the API to construct the element.
      
      - the implementation for check_normalizable is only correct, if there
        are no duplicate or overlapping ranges. Assert for that. In fact,
        all callers first check for errors and then for normalizable errors.
      
      - avoid duplicate calls to nm_bridge_vlan_get_vid_range(). There are
        duplicate assertions that we don't need.
      
      - only check for pvid once per range.
      
      - combine calls to g_hash_table_contains() and g_hash_table_add().
      a358da09
    • Beniamino Galvani's avatar
      9f23c5e2
    • Thomas Haller's avatar
      device: avoid multiple allocations in setting_vlans_to_platform() · 6bc8ee87
      Thomas Haller authored
      We don't need GPtrArray to construct an array of fixed side.
      Actually, we also don't need to malloc each NMPlatformBridgeVlan
      element individually. Just allocate one buffer and append them
      to the end.
      6bc8ee87
    • Beniamino Galvani's avatar
      all: support bridge vlan ranges · 70935157
      Beniamino Galvani authored
      In some cases it is convenient to specify ranges of bridge vlans, as
      already supported by iproute2 and natively by kernel. With this commit
      it becomes possible to add a range in this way:
      
       nmcli connection modify eth0-slave +bridge-port.vlans "100-200 untagged"
      
      vlan ranges can't be PVIDs because only one PVID vlan can exist.
      
      https://bugzilla.redhat.com/show_bug.cgi?id=1652910
      70935157
    • Beniamino Galvani's avatar
      clients: fix typos · ea16cf59
      Beniamino Galvani authored
      ea16cf59
    • Thomas Haller's avatar
      ifcfg-rh: fix compiler warning in read_routing_rules_parse() · c6e6dcae
      Thomas Haller authored
          CC       src/settings/plugins/ifcfg-rh/src_settings_plugins_ifcfg_rh_libnms_ifcfg_rh_core_la-nms-ifcfg-rh-reader.lo
        In file included from ../shared/nm-default.h:280:0,
                         from ../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:21:
        ../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c: In function read_routing_rules_parse:
        ../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:4309:27: error: logical not is only applied to the left hand side of comparison [-Werror=logical-not-parentheses]
           nm_assert (!key_is_ipv4 == NM_STR_HAS_PREFIX (key, "ROUTING_RULE6_"));
                                   ^
        ../shared/nm-utils/nm-macros-internal.h:1793:7: note: in definition of macro __NM_G_BOOLEAN_EXPR_IMPL
           if (expr) \
               ^
        /usr/include/glib-2.0/glib/gmacros.h:376:43: note: in expansion of macro _G_BOOLEAN_EXPR
         #define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR((expr)), 1))
                                                   ^
        /usr/include/glib-2.0/glib/gtestutils.h:116:49: note: in expansion of macro G_LIKELY
                                                      if G_LIKELY (expr) ; else \
                                                         ^
        ../shared/nm-utils/nm-macros-internal.h:973:40: note: in expansion of macro g_assert
         #define nm_assert(cond) G_STMT_START { g_assert (cond); } G_STMT_END
                                                ^
        ../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:4309:3: note: in expansion of macro nm_assert
           nm_assert (!key_is_ipv4 == NM_STR_HAS_PREFIX (key, "ROUTING_RULE6_"));
           ^
      
      Fixes: 4d468044
      c6e6dcae
  2. 17 Apr, 2019 7 commits
    • Rodrigo Lledó's avatar
    • Thomas Haller's avatar
      832adf32
    • Thomas Haller's avatar
    • Thomas Haller's avatar
    • Thomas Haller's avatar
      shared: remove unused nm_utils_str_simpletokens_extract_next() · ced7dbe8
      Thomas Haller authored
      This can be replaced by nm_utils_escaped_tokens_split().
      
      Note that nm_utils_escaped_tokens_split() does not behave exactly
      the same. For example, nm_utils_str_simpletokens_extract_next() would
      remove all backslashes and leave only the following character.
      nm_utils_escaped_tokens_split() instead only strips backslashes
      that preceed a delimiter, whitespace or another backslash.
      
      But we should have one preferred way of tokenizing, and I find this
      preferable, because it allows for most backslashes to appear verbatim.
      ced7dbe8
    • Thomas Haller's avatar
      cli: use nm_utils_escaped_tokens_*() for handling policy routes · d59f046b
      Thomas Haller authored
      Optimally, all list types properties in nmcli support proper escaping.
      For that, we should use nm_utils_escaped_tokens_*() API.
      
      For now, just change that for policy routes. They were just added recently,
      so no change in behavior of released API.
      d59f046b
    • Thomas Haller's avatar
      libnm: use nm_utils_escaped_tokens_*() for parsing NMIPRoutingRule · b6d0be2d
      Thomas Haller authored
      Replace nm_utils_str_simpletokens_extract_next() by
      nm_utils_escaped_tokens_split().
      
      nm_utils_escaped_tokens_split() should become our first choice for
      parsing and tokenizing.
      
      Note that both nm_utils_str_simpletokens_extract_next() and
      nm_utils_escaped_tokens_split() need to strdup the string once,
      and tokenizing takes O(n). So, they are roughtly the same performance
      wise. The only difference is, that as we iterate through the tokens,
      we might abort early on error with nm_utils_str_simpletokens_extract_next()
      and not parse the entire string. But that is a small benefit, since we
      anyway always strdup() the string (being O(n) already).
      
      Note that to-string will no longer escape ',' and ';'. This is a change
      in behavior, of unreleased API. Also note, that escaping these is no
      longer necessary, because nmcli soon will also use nm_utils_escaped_tokens_*().
      
      Another change in behavior is that nm_utils_str_simpletokens_extract_next()
      treated invalid escape sequences (backslashes followed by an arbitrary
      character), buy stripping the backslash. nm_utils_escaped_tokens_*()
      leaves such backslashes as is, and only honors them if they are followed
      by a whitespace (the delimiter) or another backslash. The disadvantage
      of the new approach is that backslashes are treated differently
      depending on the following character. The benefit is, that most
      backslashes can now be written verbatim, not requiring them to escape
      them with a double-backslash.
      
      Yes, there is a problem with these nested escape schemes:
      
        - the caller may already need to escape backslash in shell.
      
        - then nmcli will use backslash escaping to split the rules at ','.
      
        - then nm_ip_routing_rule_from_string() will honor backslash escaping
          for spaces.
      
        - then iifname and oifname use backslash escaping for nm_utils_buf_utf8safe_escape()
          to express non-UTF-8 characters (because interface names are not
          necessarily UTF-8).
      
      This is only redeamed because escaping is really only necessary for very
      unusual cases, if you want to embed a backslash, a space, a comma, or a
      non-UTF-8 character. But if you have to, now you will be able to express
      that.
      
      The other upside of these layers of escaping is that they become all
      indendent from each other:
      
        - shell can accept quoted/escaped arguments and will unescape them.
      
        - nmcli can do the tokenizing for ',' (and escape the content
          unconditionally when converting to string).
      
        - nm_ip_routing_rule_from_string() can do its tokenizing without
          special consideration of utf8safe escaping.
      
        - NMIPRoutingRule takes iifname/oifname as-is and is not concerned
          about nm_utils_buf_utf8safe_escape(). However, before configuring
          the rule in kernel, this utf8safe escape will be unescaped to get
          the interface name (which is non-UTF8 binary).
      b6d0be2d