setconfig: more thorough check for internal config options.

Now we can check that the setconfig would actually parse.  We do this
by handing NULL to the calback to indicate it's a test, which may not
work in general but works for now.

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: `check` `setconfig` now checks that the new config setting would be valid.

common/configvar.h

@@ -58,7 +58,7 @@ struct configvar {
 #define OPT_SHOWMSATS (1 << (OPT_USER_START+4))
 /* listconfigs should treat as a literal boolean `true` or `false` */
 #define OPT_SHOWBOOL (1 << (OPT_USER_START+5))
-/* Can be changed at runtime */
+/* Can be changed at runtime: cb will get called with NULL for `check`! */
 #define OPT_DYNAMIC (1 << (OPT_USER_START+6))
 
 /* Use this instead of opt_register_*_arg if you want OPT_* from above */

lightningd/configs.c

@@ -586,6 +586,7 @@ static struct command_result *json_setconfig(struct command *cmd,
 	const struct opt_table *ot;
 	const char *val;
 	char *err;
+	void *arg;
 
 	if (!param_check(cmd, buffer, params,
 			 p_req("config", param_opt_dynamic_config, &ot),
@@ -596,6 +597,12 @@ static struct command_result *json_setconfig(struct command *cmd,
 	/* We don't handle DYNAMIC MULTI, at least yet! */
 	assert(!(ot->type & OPT_MULTI));
 
+	/* We use arg = NULL to tell callback it's only for testing */
+	if (command_check_only(cmd))
+		arg = NULL;
+	else
+		arg = ot->u.arg;
+
 	if (ot->type & OPT_NOARG) {
 		if (val)
 			return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
@@ -604,10 +611,7 @@ static struct command_result *json_setconfig(struct command *cmd,
 		if (is_plugin_opt(ot))
 			return plugin_set_dynamic_opt(cmd, ot, NULL,
 						      setconfig_success);
-		/* FIXME: we don't have a check-only mode! */
-		if (command_check_only(cmd))
-			return command_check_done(cmd);
-		err = ot->cb(ot->u.arg);
+		err = ot->cb(arg);
 	} else {
 		assert(ot->type & OPT_HASARG);
 		if (!val)
@@ -617,17 +621,17 @@ static struct command_result *json_setconfig(struct command *cmd,
 		if (is_plugin_opt(ot))
 			return plugin_set_dynamic_opt(cmd, ot, val,
 						      setconfig_success);
-		/* FIXME: we don't have a check-only mode! */
-		if (command_check_only(cmd))
-			return command_check_done(cmd);
-		err = ot->cb_arg(val, ot->u.arg);
+		err = ot->cb_arg(val, arg);
 	}
 
 	if (err) {
 		return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 				    "Error setting %s: %s", ot->names + 2, err);
 	}
 
+	if (command_check_only(cmd))
+		return command_check_done(cmd);
+
 	return setconfig_success(cmd, ot, val);
 }
 

lightningd/options.c

@@ -69,6 +69,18 @@ static char *opt_set_u64(const char *arg, u64 *u)
 		return tal_fmt(tmpctx, "'%s' is out of range", arg);
 	return NULL;
 }
+
+static char *opt_set_u64_dynamic(const char *arg, u64 *u)
+{
+	u64 ignored;
+
+	/* In case we're called for arg checking only */
+	if (!u)
+		u = &ignored;
+
+	return opt_set_u64(arg, u);
+}
+
 static char *opt_set_u32(const char *arg, u32 *u)
 {
 	char *endp;
@@ -1543,9 +1555,10 @@ static void register_opts(struct lightningd *ld)
 		       opt_set_msat,
 		       opt_show_msat, &ld->config.max_dust_htlc_exposure_msat,
 		       "Max HTLC amount that can be trimmed");
-	clnopt_witharg("--min-capacity-sat", OPT_SHOWINT|OPT_DYNAMIC, opt_set_u64, opt_show_u64,
-			 &ld->config.min_capacity_sat,
-			 "Minimum capacity in satoshis for accepting channels");
+	clnopt_witharg("--min-capacity-sat", OPT_SHOWINT|OPT_DYNAMIC,
+		       opt_set_u64_dynamic, opt_show_u64,
+		       &ld->config.min_capacity_sat,
+		       "Minimum capacity in satoshis for accepting channels");
 	clnopt_witharg("--addr", OPT_MULTI, opt_add_addr, NULL,
 		       ld,
 		       "Set an IP address (v4 or v6) to listen on and announce to the network for incoming connections");

tests/test_misc.py

@@ -3782,6 +3782,22 @@ def test_setconfig(node_factory, bitcoind):
     with pytest.raises(RpcError, match='is not a number'):
         l2.rpc.setconfig(config='min-capacity-sat', val="abcd")
 
+    # Check will fail the same way.
+    with pytest.raises(RpcError, match='requires a value'):
+        l2.rpc.check('setconfig', config='min-capacity-sat')
+
+    with pytest.raises(RpcError, match='is not a number'):
+        l2.rpc.check('setconfig', config='min-capacity-sat', val="abcd")
+
+    # Check will pass, but NOT change value.
+    assert l2.rpc.check(command_to_check='setconfig', config='min-capacity-sat', val=500000) == {'command_to_check': 'setconfig'}
+
+    assert (l2.rpc.listconfigs('min-capacity-sat')['configs']
+            == {'min-capacity-sat':
+                {'source': 'default',
+                 'value_int': 10000,
+                 'dynamic': True}})
+
     ret = l2.rpc.setconfig(config='min-capacity-sat', val=500000)
     assert ret == {'config':
                    {'config': 'min-capacity-sat',