Fix new 'interfaces' config option

Introduce CFINCOMPATIBLE config parse result flag. Sort interface rules by unsigned integer key. Legacy and modern 'interfaces' config styles are now incompatible. Validate config_network_interface struct to enforce that only exactly one of 'match' and 'dummy' options are set. Add test cases for 'interface' config option.
2025-02-20 17:33:08 +00:00 · 2012-12-05 12:58:07 +10:30 · 2012-12-05 12:58:07 +10:30 · aa638a9bfd
commit aa638a9bfd
parent c53789d764
5 changed files with 195 additions and 30 deletions
--- a/conf.h
+++ b/conf.h
@ -206,8 +206,9 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 #define CFARRAYOVERFLOW     (1<<2)
 #define CFSTRINGOVERFLOW    (1<<3)
 #define CFINCOMPLETE        (1<<4)
-#define CFINVALID           (1<<5)
-#define CFUNSUPPORTED       (1<<6)
+#define CFINCOMPATIBLE      (1<<5)
+#define CFINVALID           (1<<6)
+#define CFUNSUPPORTED       (1<<7)
 #define CF__SUB_SHIFT       16
 #define CFSUB(f)            ((f) << CF__SUB_SHIFT)
 #define CF__SUBFLAGS        CFSUB(~0)
@ -268,7 +269,8 @@ void _cf_warn_no_array(struct __sourceloc __whence, const struct cf_om_node *nod
 void _cf_warn_unsupported_node(struct __sourceloc __whence, const struct cf_om_node *node);
 void _cf_warn_unsupported_children(struct __sourceloc __whence, const struct cf_om_node *parent);
 void _cf_warn_list_overflow(struct __sourceloc __whence, const struct cf_om_node *node);
-void _cf_warn_spurious_children(struct __sourceloc __whence, const struct cf_om_node *parent);
+void _cf_warn_incompatible(struct __sourceloc __whence, const struct cf_om_node *node, const struct cf_om_node *orig);
+void _cf_warn_incompatible_children(struct __sourceloc __whence, const struct cf_om_node *parent);
 void _cf_warn_array_key(struct __sourceloc __whence, const struct cf_om_node *node, int reason);
 void _cf_warn_array_value(struct __sourceloc __whence, const struct cf_om_node *node, int reason);

@ -283,7 +285,8 @@ void _cf_warn_array_value(struct __sourceloc __whence, const struct cf_om_node *
 #define cf_warn_unsupported_node(node)	     _cf_warn_unsupported_node(__WHENCE__, node)
 #define cf_warn_unsupported_children(parent) _cf_warn_unsupported_children(__WHENCE__, parent)
 #define cf_warn_list_overflow(node)	     _cf_warn_list_overflow(__WHENCE__, node)
-#define cf_warn_spurious_children(parent)    _cf_warn_spurious_children(__WHENCE__, parent)
+#define cf_warn_incompatible(node, orig)     _cf_warn_incompatible(__WHENCE__, node, orig)
+#define cf_warn_incompatible_children(parent) _cf_warn_incompatible_children(__WHENCE__, parent)
 #define cf_warn_array_key(node, reason)	     _cf_warn_array_key(__WHENCE__, node, reason)
 #define cf_warn_array_value(node, reason)    _cf_warn_array_value(__WHENCE__, node, reason)

--- a/conf_om.c
+++ b/conf_om.c
@ -383,9 +383,26 @@ void _cf_warn_missing_node(struct __sourceloc __whence, const struct cf_om_node
  _cf_warn_node(__whence, parent, key, "is missing");
 }

-void _cf_warn_spurious_children(struct __sourceloc __whence, const struct cf_om_node *parent)
+void _cf_warn_incompatible(struct __sourceloc __whence, const struct cf_om_node *node, const struct cf_om_node *orig)
 {
-  _cf_warn_children(__whence, parent, "spurious");
+  assert(node != orig);
+  strbuf b = strbuf_alloca(180);
+  if (orig) {
+    strbuf_sprintf(b, "\"%s\"", orig->fullkey);
+    if (orig->source && orig->line_number)
+      strbuf_sprintf(b, " at %s:%u", orig->source, orig->line_number);
+  } else {
+    strbuf_puts(b, "other option(s)");
+  }
+  _cf_warn_node(__whence, node, NULL, "is incompatible with %s", strbuf_str(b));
+}
+
+void _cf_warn_incompatible_children(struct __sourceloc __whence, const struct cf_om_node *parent)
+{
+  struct cf_om_iterator it;
+  for (cf_om_iter_start(&it, parent); it.node; cf_om_iter_next(&it))
+    if (it.node != parent && it.node->text)
+      _cf_warn_incompatible(__whence, parent, it.node);
 }

 void _cf_warn_unsupported_node(struct __sourceloc __whence, const struct cf_om_node *node)
@ -414,6 +431,7 @@ strbuf strbuf_cf_flags(strbuf sb, int flags)
      { CFSTRINGOVERFLOW, "CFSTRINGOVERFLOW" },
      { CFARRAYOVERFLOW, "CFARRAYOVERFLOW" },
      { CFINCOMPLETE, "CFINCOMPLETE" },
+      { CFINCOMPATIBLE, "CFINCOMPATIBLE" },
      { CFINVALID, "CFINVALID" },
      { CFUNSUPPORTED, "CFUNSUPPORTED" },
    };
@ -457,6 +475,7 @@ strbuf strbuf_cf_flag_reason(strbuf sb, int flags)
      { CFSTRINGOVERFLOW, "string overflow" },
      { CFARRAYOVERFLOW, "array overflow" },
      { CFINCOMPLETE, "incomplete" },
+      { CFINCOMPATIBLE, "incompatible" },
      { CFINVALID, "invalid" },
      { CFUNSUPPORTED, "not supported" },
      { CFSUB(CFEMPTY), "contains empty element" },
@ -464,6 +483,7 @@ strbuf strbuf_cf_flag_reason(strbuf sb, int flags)
      { CFSUB(CFSTRINGOVERFLOW), "contains string overflow" },
      { CFSUB(CFARRAYOVERFLOW), "contains array overflow" },
      { CFSUB(CFINCOMPLETE), "contains incomplete element" },
+      { CFSUB(CFINCOMPATIBLE), "contains incompatible element" },
      { CFSUB(CFINVALID), "contains invalid element" },
      { CFSUB(CFUNSUPPORTED), "contains unsupported element" },
    };
--- a/conf_schema.c
+++ b/conf_schema.c
@ -19,6 +19,7 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.

 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <errno.h>
 #include <sys/types.h>
 #include <stdarg.h>
@ -121,7 +122,10 @@ int cf_opt_rhizome_peer(struct config_rhizome_peer *rpeer, const struct cf_om_no
 {
  if (!node->text)
    return cf_opt_config_rhizome_peer(rpeer, node);
-  cf_warn_spurious_children(node);
+  if (node->nodc) {
+    cf_warn_incompatible_children(node);
+    return CFINCOMPATIBLE;
+  }
  return cf_opt_rhizome_peer_from_uri(rpeer, node->text);
 }

@ -184,6 +188,16 @@ int cf_opt_int(int *intp, const char *text)
  return CFOK;
 }

+int cf_opt_uint(unsigned int *uintp, const char *text)
+{
+  const char *end = text;
+  unsigned long value = strtoul(text, (char**)&end, 10);
+  if (end == text || *end)
+    return CFINVALID;
+  *uintp = value;
+  return CFOK;
+}
+
 int cf_opt_int32_nonneg(int32_t *intp, const char *text)
 {
  const char *end = text;
@ -398,6 +412,7 @@ static int cf_opt_network_interface_legacy(struct config_network_interface *nifp
 {
  //DEBUGF("%s text=%s", __FUNCTION__, alloca_str_toprint(text));
  struct config_network_interface nif;
+  (&nif);
  cf_dfl_config_network_interface(&nif);
  if (text[0] != '+' && text[0] != '-')
    return CFINVALID; // "Sign must be + or -"
@ -408,13 +423,20 @@ static int cf_opt_network_interface_legacy(struct config_network_interface *nifp
  if (!p)
    p = endtext;
  size_t len = p - name;
-  int star = (len == 0 || (name[0] != '>' && name[len - 1] != '*')) ? 1 : 0;
-  if (len + star >= sizeof(nif.match.patv[0]))
-    return CFSTRINGOVERFLOW;
-  strncpy(nif.match.patv[0], name, len)[len + star] = '\0';
-  if (star)
-    nif.match.patv[0][len] = '*';
-  nif.match.patc = 1;
+  if (name[0] == '>') {
+    if (len - 1 >= sizeof(nif.match.patv[0]))
+      return CFSTRINGOVERFLOW;
+    strncpy(nif.dummy, &name[1], len - 1)[len - 1] = '\0';
+    nif.match.patc = 0;
+  } else {
+    int star = (strchr(name, '*') != NULL) ? 1 : 0;
+    if (len + star >= sizeof(nif.match.patv[0]))
+      return CFSTRINGOVERFLOW;
+    strncpy(nif.match.patv[0], name, len)[len + star] = '\0';
+    if (star)
+      nif.match.patv[0][len] = '*';
+    nif.match.patc = 1;
+  }
  if (*p == '=') {
    const char *const type = p + 1;
    p = strchr(type, ':');
@ -476,10 +498,31 @@ int cf_opt_network_interface(struct config_network_interface *nifp, const struct
 {
  if (!node->text)
    return cf_opt_config_network_interface(nifp, node);
-  cf_warn_spurious_children(node);
+  if (node->nodc) {
+    cf_warn_incompatible_children(node);
+    return CFINCOMPATIBLE;
+  }
  return cf_opt_network_interface_legacy(nifp, node->text);
 }

+int vld_network_interface(const struct cf_om_node *parent, struct config_network_interface *nifp, int result)
+{
+  if (nifp->match.patc != 0 && nifp->dummy[0]) {
+    int nodei_match = cf_om_get_child(parent, "match", NULL);
+    int nodei_dummy = cf_om_get_child(parent, "dummy", NULL);
+    assert(nodei_match != -1);
+    assert(nodei_dummy != -1);
+    cf_warn_incompatible(parent->nodv[nodei_match], parent->nodv[nodei_dummy]);
+    return result | CFSUB(CFINCOMPATIBLE);
+  }
+  if (nifp->match.patc == 0 && !nifp->dummy[0]) {
+    DEBUGF("dummy=%s", alloca_str_toprint(nifp->dummy));
+    cf_warn_missing_node(parent, "match");
+    return result | CFINCOMPLETE;
+  }
+  return result;
+}
+
 /* Config parse function.  Implements the original form of the 'interfaces' config option.  Parses a
 * comma-separated list of interface rules (see cf_opt_network_interface_legacy() for the format of
 * each rule), then parses the regular config array-of-struct style interface option settings so
@ -491,6 +534,10 @@ int cf_opt_interface_list(struct config_interface_list *listp, const struct cf_o
 {
  if (!node->text)
    return cf_opt_config_interface_list(listp, node);
+  if (node->nodc) {
+    cf_warn_incompatible_children(node);
+    return CFINCOMPATIBLE;
+  }
  const char *p;
  const char *arg = NULL;
  unsigned n = listp->ac;
@ -509,8 +556,7 @@ int cf_opt_interface_list(struct config_interface_list *listp, const struct cf_o
 	switch (ret) {
 	case CFERROR: return CFERROR;
 	case CFOK:
-	  len = snprintf(listp->av[n].key, sizeof listp->av[n].key - 1, "%u", n);
-	  listp->av[n].key[len] = '\0';
+	  listp->av[n].key = n;
 	  ++n;
 	  break;
 	default:
--- a/conf_schema.h
+++ b/conf_schema.h
@ -285,9 +285,9 @@ KEY_ATOM(sid_t, cf_opt_sid, cmp_sid)
 VALUE_SUB_STRUCT(host)
 END_ARRAY(32)

-STRUCT(network_interface)
+STRUCT(network_interface, vld_network_interface)
 ATOM(int,                   exclude,    0, cf_opt_boolean,, "If true, do not use matching interfaces")
-ATOM(struct pattern_list,   match,      PATTERN_LIST_EMPTY, cf_opt_pattern_list, MANDATORY, "Names that match network interface")
+ATOM(struct pattern_list,   match,      PATTERN_LIST_EMPTY, cf_opt_pattern_list,, "Names that match network interface")
 STRING(256,                 dummy,      "", cf_opt_str_nonempty,, "Path of dummy file, absolute or relative to server.dummy_interface_dir")
 ATOM(short,                 type,       OVERLAY_INTERFACE_WIFI, cf_opt_interface_type,, "Type of network interface")
 ATOM(uint16_t,              port,       RHIZOME_HTTP_PORT, cf_opt_port,, "Port number for network interface")
@ -295,8 +295,8 @@ ATOM(uint64_t,              speed,      1000000, cf_opt_uint64_scaled,, "Speed i
 ATOM(int,                   mdp_tick_ms, -1, cf_opt_int32_nonneg,, "Override MDP tick interval for this interface")
 END_STRUCT

-ARRAY(interface_list,)
-KEY_STRING(15, cf_opt_str)
+ARRAY(interface_list, SORTED NO_DUPLICATES)
+KEY_ATOM(unsigned, cf_opt_uint)
 VALUE_NODE_STRUCT(network_interface, cf_opt_network_interface)
 END_ARRAY(10)

--- a/tests/config
+++ b/tests/config
@ -25,10 +25,31 @@ setup() {
   setup_servald
 }

-execute_servald_config_error() {
-   execute --stderr --exit-status=2 --core-backtrace --executable=$servald "$@"
-   assertStderrGrep --matches=1 --message="stderr of ($executed) contains exactly one error message" '^ERROR:'
-   assertStderrGrep --matches=1 --message="stderr of ($executed) contains one config error message" '^ERROR:.*config file'
+assert_stderr_log() {
+   local -a warn_patterns=()
+   local -a error_patterns=()
+   while [ $# -gt 0 ]; do
+      case "$1" in
+      --warn-pattern=*) warn_patterns+=("${1#*=}"); shift;;
+      --error-pattern=*) error_patterns+=("${1#*=}"); shift;;
+      *) error "unsupported option: $1"; return;;
+      esac
+   done
+   assertStderrGrep \
+      --matches=${#error_patterns[*]} \
+      --message="stderr of ($executed) contains exactly ${#error_patterns[*]} error message(s)" \
+      '^ERROR:'
+   local pattern
+   for pattern in "${warn_patterns[@]}"; do
+      assertStderrGrep \
+         --message="stderr of ($executed) contains a warning message matching \"$pattern\"" \
+         "^WARN:.*$pattern"
+   done
+   for pattern in "${error_patterns[@]}"; do
+      assertStderrGrep \
+         --message="stderr of ($executed) contains an error message matching \"$pattern\"" \
+         "^ERROR:.*$pattern"
+   done
 }

 doc_GetCreateInstanceDir="Get creates instance directory"
@ -154,14 +175,89 @@ test_DebugFlagAll() {
   assertStderrGrep --matches=3 '\<echo:argv\['
 }

-doc_InterfacesLegacy="Legacy interfaces config option is supported"
-test_InterfacesLegacy() {
-   executeOk_servald config set interfaces '+eth,-wifi,+'
+doc_InterfacesLegacyIncludeAll="Legacy 'interfaces' config option include all"
+test_InterfacesLegacyIncludeAll() {
   executeOk_servald config set interfaces '+'
+}
+
+doc_InterfacesLegacyExcludeAll="Legacy 'interfaces' config option exclude all"
+test_InterfacesLegacyExcludeAll() {
   executeOk_servald config set interfaces '-'
+}
+
+doc_InterfacesLegacyMixed="Legacy 'interfaces' config option mixed list"
+test_InterfacesLegacyMixed() {
+   executeOk_servald config set interfaces '+eth,-wifi,+'
+}
+
+doc_InterfacesLegacyMixedDetail="Legacy 'interfaces' config option mixed list with details"
+test_InterfacesLegacyMixedDetail() {
   executeOk_servald config set interfaces '+eth=ethernet:4111:9M, +wifi=wifi:4112:900K, -'
-   execute_servald_config_error config set interfaces '+eth=foo:4111:9M'
-   assertExitStatus --stderr '!=' 0
+}
+
+doc_InterfacesLegacyInvalidType="Legacy 'interfaces' config option invalid type"
+test_InterfacesLegacyInvalidType() {
+   execute --stderr --core-backtrace --exit-status=2 --executable=$servald \
+      config set interfaces '+eth=foo:4111:9M'
+   assert_stderr_log \
+      --warn-pattern='"interfaces".*invalid' \
+      --error-pattern='config file.*not loaded.*invalid'
+}
+
+doc_InterfacesLegacyInvalidPort="Legacy 'interfaces' config option invalid port"
+test_InterfacesLegacyInvalidPort() {
+   execute --stderr --core-backtrace --exit-status=2 --executable=$servald \
+      config set interfaces '+eth=ethernet:-1000:9M'
+   assert_stderr_log \
+      --warn-pattern='"interfaces".*invalid' \
+      --error-pattern='config file.*not loaded.*invalid'
+}
+
+doc_InterfacesLegacyInvalidSpeed="Legacy 'interfaces' config option invalid speed"
+test_InterfacesLegacyInvalidSpeed() {
+   execute --stderr --core-backtrace --exit-status=2 --executable=$servald \
+      config set interfaces '+eth=ethernet:4111:9MB'
+   assert_stderr_log \
+      --warn-pattern='"interfaces".*invalid' \
+      --error-pattern='config file.*not loaded.*invalid'
+}
+
+doc_InterfacesLegacyIncompatible="Legacy 'interfaces' config option incompatible with modern form"
+test_InterfacesLegacyIncompatible() {
+   executeOk_servald config set interfaces '+'
+   execute --stderr --core-backtrace --executable=$servald \
+      config set interfaces.10.match 'eth'
+   tfw_cat $SERVALINSTANCE_PATH/serval.conf
+   assertExitStatus '==' 2
+   assert_stderr_log \
+      --warn-pattern='"interfaces.*incompatible' \
+      --error-pattern='config file.*not loaded.*incompatible'
+   tfw_cat --stderr
+}
+
+doc_InterfacesModernMatch="Modern 'interfaces.N.match' config option"
+test_InterfacesModernMatch() {
+   executeOk_servald config set interfaces.0.match 'eth*, wifi*'
+}
+
+doc_InterfacesModernDummy="Modern 'interfaces.N.dummy' config option"
+test_InterfacesModernDummy() {
+   executeOk_servald config set interfaces.0.dummy dummyname
+}
+teardown_InterfacesModernDummy() {
+   tfw_cat $SERVALINSTANCE_PATH/serval.conf
+   teardown
+}
+
+doc_InterfacesModernIncompatible="Config options 'interfaces.match' and 'interfaces.dummy' are incompatible"
+test_InterfacesModernIncompatible() {
+   executeOk_servald config set interfaces.0.match eth
+   execute --stderr --core-backtrace --executable=$servald \
+      config set interfaces.0.dummy dummy
+   assertExitStatus '==' 2
+   assert_stderr_log \
+      --warn-pattern='"interfaces\.0\..*incompatible' \
+      --error-pattern='config file.*not loaded.*incompatible'
 }

 runTests "$@"