Add comments to try to prevent recurrence of #31495.

There is a bad design choice in two of our configuration types,
where the empty string encodes a value that is not the same as the
default value.  This design choice, plus an implementation mistake,
meant that config_dup() did not preserve the value of routerset_t,
and thereby caused bug #31495.

This comment-only patch documents the two types with the problem,
and suggests that implementors try to avoid it in the future.

Closes ticket 31907.

1 files changed, 7 insertions, 0 deletions

diff --git a/src/feature/nodelist/routerset.c b/src/feature/nodelist/routerset.c
index 73c2b1b1de..9a205d39b7 100644
--- a/src/feature/nodelist/routerset.c
+++ b/src/feature/nodelist/routerset.c
@@ -473,6 +473,13 @@ routerset_free_(routerset_t *routerset)
  * routerset_t** passed as <b>target</b>.  On success return 0; on failure
  * return -1 and store an error message into *<b>errmsg</b>.
  **/
+/*
+ * Warning: For this type, the default value (NULL) and "" are sometimes
+ * considered different values.  That is generally risky, and best avoided for
+ * other types in the future.  For cases where we want the default to be "all
+ * routers" (like EntryNodes) we should add a new routerset value indicating
+ * "all routers" (see #31908)
+ */
 static int
 routerset_kv_parse(void *target, const config_line_t *line, char **errmsg,
                   const void *params)