[Swan-dev] puzzled by addcon's use of resolve_defaultroute

[D. Hugh Redelmeier]

The use of resolve_defaultroute() in file programs/addconn/addconn.c
is the focus of my concern.

Have a look at at 389bd0f3e66774f0da63d23653cbdf85a9ceb2d1
(I've included it at the bottom for your convenience.)

1) after the commit resolve_defaultroute does nothing if HAVE_NETKEY is 
   undefined.
   (Before the commit it wasn't defined if HAVE_NETKEY were undefined.)

2) yet resolve_defaultroute is called, even if HAVE_NETKEY is
   undefined.  This makes the code misleading since the contract
   implied by its name is that it will actually resolve default route.

So I don't like this commit, even though it does cut down #ifdefs.
This could be remedied by renaming the function to something like
resolve_defaultroute_if_netkey.  But I think that this is no better
than the original form, with an extra #ifdef.

But wait!  Even before this patch, there were two calls to
resolve_defaultroute that were not wrapped in #ifdef HAVE_NETKEY.  How
did that even compile if HAVE_NETKEY were undefined?

Perhaps HAVE_NETKEY has always been defined and we should just presume
it everywhere?

Hmmm.  Actually, the ifdefs came in with
1e7d5f86554dbdc563ed024279cf102c1e842d1e
(only 9 hours earlier than 389bd0f3e66774f0da63d23653cbdf85a9ceb2d1).
Clearly the code was never compiled without HAVE_NETKEY to test this
change: neither clean compiling nor actually functioning.

So:

1) Does the code work without HAVE_NETKEY?
   (Did it work before the two commits?)
   If it does, why are the resolve_defaultroute calls needed in the
   HAVE_NETKEY case?

2) how can we rename resolve_defaultroute so as not to be misleading?
   Alternatively we could put the ifdefs around each call.

================================================================

commit 389bd0f3e66774f0da63d23653cbdf85a9ceb2d1
Author: Andrew Cagney <cagney at gnu.org>
Date:   Wed Jul 18 17:14:50 2018 -0400

    addconn: simplify check for HAVE_NETKEY

diff --git a/programs/addconn/addconn.c b/programs/addconn/addconn.c
index e85815e4d..dc8f7a577 100644
--- a/programs/addconn/addconn.c
+++ b/programs/addconn/addconn.c
@@ -55,7 +55,10 @@
 #include "ipsecconf/starterwhack.h"
 #include "ipsecconf/keywords.h"
 #include "ipsecconf/parser-controls.h"
+
+#ifdef HAVE_NETKEY
 #include "addr_lookup.h"
+#endif
 
 #ifdef USE_DNSSEC
 # include "dnssec.h"
@@ -76,16 +79,16 @@ static int verbose = 0;
  * XXX: why not let pluto resolve all this like it is already doing
  * because of MOBIKE.
  */
-#ifdef HAVE_NETKEY
 static
-void resolve_defaultroute(struct starter_conn *conn)
+void resolve_defaultroute(struct starter_conn *conn UNUSED)
 {
+#ifdef HAVE_NETKEY
 	if (resolve_defaultroute_one(&conn->left, &conn->right, verbose != 0) == 1)
 		resolve_defaultroute_one(&conn->left, &conn->right, verbose != 0);
 	if (resolve_defaultroute_one(&conn->right, &conn->left, verbose != 0) == 1)
 		resolve_defaultroute_one(&conn->right, &conn->left, verbose != 0);
-}
 #endif
+}
 
 #ifdef HAVE_SECCOMP
 static void init_seccomp_addconn(uint32_t def_action)
@@ -448,9 +451,7 @@ int main(int argc, char *argv[])
 			{
 				if (verbose)
 					printf(" %s", conn->name);
-#ifdef HAVE_NETKEY
 				resolve_defaultroute(conn);
-#endif
 				starter_whack_add_conn(cfg, conn);
 			}
 		}
@@ -545,8 +546,7 @@ int main(int argc, char *argv[])
 								connname,
 								conn->name);
 						} else {
-							resolve_defaultroute(
-								conn);
+							resolve_defaultroute(conn);
 							exit_status =
 								starter_whack_add_conn(
 									cfg,