From 4b081f6c97bed741346de518200fbe2e1b2a7e71 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 20 Mar 2024 11:05:28 +1030 Subject: [PATCH] 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 Changelog-Added: `check` `setconfig` now checks that the new config setting would be valid. --- common/configvar.h | 2 +- lightningd/configs.c | 20 ++++++++++++-------- lightningd/options.c | 19 ++++++++++++++++--- tests/test_misc.py | 16 ++++++++++++++++ 4 files changed, 45 insertions(+), 12 deletions(-) diff --git a/common/configvar.h b/common/configvar.h index 81ffda172960..9a308ecd5b11 100644 --- a/common/configvar.h +++ b/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 */ diff --git a/lightningd/configs.c b/lightningd/configs.c index 5612bcd91e37..dd6b5048e125 100644 --- a/lightningd/configs.c +++ b/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,10 +621,7 @@ 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) { @@ -628,6 +629,9 @@ static struct command_result *json_setconfig(struct command *cmd, "Error setting %s: %s", ot->names + 2, err); } + if (command_check_only(cmd)) + return command_check_done(cmd); + return setconfig_success(cmd, ot, val); } diff --git a/lightningd/options.c b/lightningd/options.c index 0851ef8ad87b..5609e66dc6e0 100644 --- a/lightningd/options.c +++ b/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"); diff --git a/tests/test_misc.py b/tests/test_misc.py index 37d2c023c39c..994425ecfd2c 100644 --- a/tests/test_misc.py +++ b/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',