From 39640728c332980daf7ca639827735a1c359669a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 1 Oct 2019 09:42:10 -0400 Subject: 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. --- src/feature/nodelist/routerset.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'src/feature/nodelist/routerset.c') 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 target. On success return 0; on failure * return -1 and store an error message into *errmsg. **/ +/* + * 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) -- cgit v1.2.3-54-g00ecf