From c2a53b110074543c70f178677b69c90d4168fb26 Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:05:57 -0600 Subject: [PATCH 1/7] pyln-testing: save stderr logs for checking just for convenience's sake --- contrib/pyln-testing/pyln/testing/utils.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index a452d1f6b250..62087ecf3ee5 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -140,6 +140,7 @@ def __init__(self, outputDir=None, verbose=True): self.proc = None self.outputDir = outputDir self.logsearch_start = 0 + self.err_logs = [] # Should we be logging lines we read from stdout? self.verbose = verbose @@ -210,6 +211,10 @@ def tail(self): self.running = False self.proc.stdout.close() if self.proc.stderr: + for line in iter(self.proc.stderr.readline, ''): + if len(line) == 0: + break + self.err_logs.append(line.rstrip().decode('ASCII')) self.proc.stderr.close() def is_in_log(self, regex, start=0): @@ -224,6 +229,18 @@ def is_in_log(self, regex, start=0): logging.debug("Did not find '%s' in logs", regex) return None + def is_in_stderr(self, regex): + """Look for `regex` in stderr.""" + + ex = re.compile(regex) + for l in self.err_logs: + if ex.search(l): + logging.debug("Found '%s' in stderr", regex) + return l + + logging.debug("Did not find '%s' in stderr", regex) + return None + def wait_for_logs(self, regexs, timeout=TIMEOUT): """Look for `regexs` in the logs. @@ -637,8 +654,8 @@ def is_synced_with_bitcoin(self, info=None): info = self.rpc.getinfo() return 'warning_bitcoind_sync' not in info and 'warning_lightningd_sync' not in info - def start(self, wait_for_bitcoind_sync=True): - self.daemon.start() + def start(self, wait_for_bitcoind_sync=True, stderr=None): + self.daemon.start(stderr=stderr) # Cache `getinfo`, we'll be using it a lot self.info = self.rpc.getinfo() # This shortcut is sufficient for our simple tests. From ba49d036e4de2a0cf1c8707d6dea0a3a716a6344 Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:06:43 -0600 Subject: [PATCH 2/7] pyln-testing: add flag 'expect_fail' to node factory get_node if the node fails to start (and we're expecting it to) return to us the node object anyway we also signal to collect all of its stderr logs by setting stderr on the tailableproc that backs the node --- contrib/pyln-testing/pyln/testing/utils.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index 62087ecf3ee5..79351ec55d71 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -990,7 +990,7 @@ def get_nodes(self, num_nodes, opts=None): def get_node(self, node_id=None, options=None, dbfile=None, feerates=(15000, 7500, 3750), start=True, - wait_for_bitcoind_sync=True, **kwargs): + wait_for_bitcoind_sync=True, expect_fail=False, **kwargs): node_id = self.get_node_id() if not node_id else node_id port = self.get_next_port() @@ -1021,8 +1021,15 @@ def get_node(self, node_id=None, options=None, dbfile=None, if start: try: - node.start(wait_for_bitcoind_sync) + # Capture stderr if we're failing + if expect_fail: + stderr = subprocess.PIPE + else: + stderr = None + node.start(wait_for_bitcoind_sync, stderr=stderr) except Exception: + if expect_fail: + return node node.daemon.stop() raise return node From a8e93245b7662349ccb250871e2c4523af2a6a9d Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:10:20 -0600 Subject: [PATCH 3/7] plugins: pass back opts as indicated type. fixes #3577 Changelog-Fixed: Plugins: if an option has a type int or bool, return the option as that type to the plugin's init --- lightningd/plugin.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 2db21cc9ef39..48ac879d229b 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -1102,6 +1102,14 @@ plugin_populate_init_request(struct plugin *plugin, struct jsonrpc_request *req) list_for_each(&plugin->plugin_opts, opt, list) { /* Trim the `--` that we added before */ name = opt->name + 2; + if (opt->value->as_bool) { + json_add_bool(req->stream, name, *opt->value->as_bool); + continue; + } + if (opt->value->as_int) { + json_add_s64(req->stream, name, *opt->value->as_int); + continue; + } if (opt->value->as_str) { json_add_string(req->stream, name, opt->value->as_str); } From a413dc5f840f053cd48f81471cf3c9de9b29f2bc Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:11:32 -0600 Subject: [PATCH 4/7] pyln: enforce types of options we loosely enforce that the specified type must be one of the listed options. you can still cause an error because we're not checking the default value you're passing in ... not sure if this is totally necessary, should we jsut let clightning enforce the input? --- contrib/pyln-client/pyln/client/plugin.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/pyln-client/pyln/client/plugin.py b/contrib/pyln-client/pyln/client/plugin.py index 5ed9ed0da10a..dbc1cbdd0bd9 100644 --- a/contrib/pyln-client/pyln/client/plugin.py +++ b/contrib/pyln-client/pyln/client/plugin.py @@ -250,6 +250,9 @@ def add_option(self, name, default, description, opt_type="string"): "Name {} is already used by another option".format(name) ) + if opt_type not in ["string", "int", "bool"]: + raise ValueError('{} not in supported type set (string, int, bool)') + self.options[name] = { 'name': name, 'default': default, From b1b8fcb6423d2df4e2c61d87db6bce8a52b573e2 Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:13:12 -0600 Subject: [PATCH 5/7] plugins: use stricter parsing for option values also: convert the stored int value from 'int' to 's64' atoi fails silently, returning a zero. instead we use the more robust strtoll which will allow us fail with an error. we also make the parsing for bools stricter, only allowing plausibly boolean values to parse. --- lightningd/plugin.c | 46 ++++++++++++++++++++++++++++++++++----------- lightningd/plugin.h | 2 +- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 48ac879d229b..35c736e0db98 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -466,13 +466,37 @@ struct io_plan *plugin_stdout_conn_init(struct io_conn *conn, char *plugin_opt_set(const char *arg, struct plugin_opt *popt) { + char *endp; + long long l; + tal_free(popt->value->as_str); + popt->value->as_str = tal_strdup(popt, arg); - if (streq(popt->type, "int")) - *popt->value->as_int = atoi(arg); - else if (streq(popt->type, "bool")) - *popt->value->as_bool = streq(arg, "true") || streq(arg, "True") - || streq(arg, "1"); + if (streq(popt->type, "int")) { + errno = 0; + l = strtoll(arg, &endp, 0); + if (errno || *endp) + return tal_fmt(tmpctx, "%s does not parse as type %s", + popt->value->as_str, popt->type); + *popt->value->as_int = l; + + /* Check if the number did not fit in `s64` (in case `long long` + * is a bigger type). */ + if (*popt->value->as_int != l) + return tal_fmt(tmpctx, "%s does not parse as type %s (overflowed)", + popt->value->as_str, popt->type); + } else if (streq(popt->type, "bool")) { + /* valid values are 'true', 'True', '1', '0', 'false', 'False', or '' */ + if (streq(arg, "true") || streq(arg, "True") || streq(arg, "1")) { + *popt->value->as_bool = true; + } else if (streq(arg, "false") || streq(arg, "False") + || streq(arg, "0") || streq(arg, "")) { + *popt->value->as_bool = false; + } else + return tal_fmt(tmpctx, "%s does not parse as type %s", + popt->value->as_str, popt->type); + } + return NULL; } @@ -509,12 +533,12 @@ static bool plugin_opt_add(struct plugin *plugin, const char *buffer, } } else if (json_tok_streq(buffer, typetok, "int")) { popt->type = "int"; - popt->value->as_int = talz(popt->value, int); + popt->value->as_int = talz(popt->value, s64); if (defaulttok) { - json_to_int(buffer, defaulttok, popt->value->as_int); - popt->value->as_str = tal_fmt(popt->value, "%d", *popt->value->as_int); + json_to_s64(buffer, defaulttok, popt->value->as_int); + popt->value->as_str = tal_fmt(popt->value, "%"PRIu64, *popt->value->as_int); popt->description = tal_fmt( - popt, "%.*s (default: %i)", desctok->end - desctok->start, + popt, "%.*s (default: %"PRIu64")", desctok->end - desctok->start, buffer + desctok->start, *popt->value->as_int); } } else if (json_tok_streq(buffer, typetok, "bool")) { @@ -535,7 +559,7 @@ static bool plugin_opt_add(struct plugin *plugin, const char *buffer, popt->description = json_strdup(popt, buffer, desctok); list_add_tail(&plugin->plugin_opts, &popt->list); opt_register_arg(popt->name, plugin_opt_set, NULL, popt, - popt->description); + popt->description); return true; } @@ -1185,7 +1209,7 @@ void json_add_opt_plugins(struct json_stream *response, if (opt->value->as_bool) { json_add_bool(response, opt_name, opt->value->as_bool); } else if (opt->value->as_int) { - json_add_s32(response, opt_name, *opt->value->as_int); + json_add_s64(response, opt_name, *opt->value->as_int); } else if (opt->value->as_str) { json_add_string(response, opt_name, opt->value->as_str); } else { diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 97ba82e3bcf0..350115adad90 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -114,7 +114,7 @@ struct plugins { */ struct plugin_opt_value { char *as_str; - int *as_int; + s64 *as_int; bool *as_bool; }; From dc708672065151b6812927d26eb75f5031236aae Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:15:37 -0600 Subject: [PATCH 6/7] plugins: test for option value checking and parsing --- tests/plugins/options.py | 20 ++++++++++++++++ tests/test_plugin.py | 50 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100755 tests/plugins/options.py diff --git a/tests/plugins/options.py b/tests/plugins/options.py new file mode 100755 index 000000000000..cb08d0b0bb8b --- /dev/null +++ b/tests/plugins/options.py @@ -0,0 +1,20 @@ +#!/usr/bin/env python3 +"""This plugin is used to check that plugin options are parsed properly. + +The plugin offers 3 options, one of each supported type. +""" +from lightning import Plugin + +plugin = Plugin() + + +@plugin.init() +def init(configuration, options, plugin): + for name, val in options.items(): + plugin.log("option {} {} {}".format(name, val, type(val))) + + +plugin.add_option('str_opt', 'i am a string', 'an example string option') +plugin.add_option('int_opt', 7, 'an example int type option', opt_type='int') +plugin.add_option('bool_opt', True, 'an example bool type option', opt_type='bool') +plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 6351ba7f2a84..f3eebbbfd1b0 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -46,6 +46,56 @@ def test_option_passthrough(node_factory, directory): n.stop() +def test_option_types(node_factory): + """Ensure that desired types of options are + respected in output """ + + plugin_path = os.path.join(os.getcwd(), 'tests/plugins/options.py') + n = node_factory.get_node(options={ + 'plugin': plugin_path, + 'str_opt': 'ok', + 'int_opt': 22, + 'bool_opt': 1, + }) + + n.daemon.is_in_log(r"option str_opt ok ") + n.daemon.is_in_log(r"option int_opt 22 ") + n.daemon.is_in_log(r"option bool_opt True ") + n.stop() + + # A blank bool_opt should default to false + n = node_factory.get_node(options={ + 'plugin': plugin_path, 'str_opt': 'ok', + 'int_opt': 22, + 'bool_opt': '', + }) + + n.daemon.is_in_log(r"option bool_opt False ") + n.stop() + + # What happens if we give it a bad bool-option? + n = node_factory.get_node(options={ + 'plugin': plugin_path, + 'str_opt': 'ok', + 'int_opt': 22, + 'bool_opt': '!', + }, expect_fail=True, may_fail=True) + + # the node should fail to start, and we get a stderr msg + assert n.daemon.is_in_stderr('bool_opt: ! does not parse as type bool') + + # What happens if we give it a bad int-option? + n = node_factory.get_node(options={ + 'plugin': plugin_path, + 'str_opt': 'ok', + 'int_opt': 'notok', + 'bool_opt': 1, + }, may_fail=True, expect_fail=True) + + # the node should fail to start, and we get a stderr msg + assert n.daemon.is_in_stderr('--int_opt: notok does not parse as type int') + + def test_millisatoshi_passthrough(node_factory): """ Ensure that Millisatoshi arguments and return work. """ From e9239d93ae5d74beacd12f4d0f9377073c4dc56e Mon Sep 17 00:00:00 2001 From: lisa neigut Date: Sat, 7 Mar 2020 18:24:47 -0600 Subject: [PATCH 7/7] plugin: add in deprecated_api behavior and test we also check that the node isn't running now, for extra pedancity --- lightningd/plugin.c | 6 ++++-- tests/test_plugin.py | 19 ++++++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 35c736e0db98..04948a0ce41a 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -1128,11 +1128,13 @@ plugin_populate_init_request(struct plugin *plugin, struct jsonrpc_request *req) name = opt->name + 2; if (opt->value->as_bool) { json_add_bool(req->stream, name, *opt->value->as_bool); - continue; + if (!deprecated_apis) + continue; } if (opt->value->as_int) { json_add_s64(req->stream, name, *opt->value->as_int); - continue; + if (!deprecated_apis) + continue; } if (opt->value->as_str) { json_add_string(req->stream, name, opt->value->as_str); diff --git a/tests/test_plugin.py b/tests/test_plugin.py index f3eebbbfd1b0..440b0dd4575c 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -55,7 +55,7 @@ def test_option_types(node_factory): 'plugin': plugin_path, 'str_opt': 'ok', 'int_opt': 22, - 'bool_opt': 1, + 'bool_opt': True, }) n.daemon.is_in_log(r"option str_opt ok ") @@ -82,6 +82,7 @@ def test_option_types(node_factory): }, expect_fail=True, may_fail=True) # the node should fail to start, and we get a stderr msg + assert not n.daemon.running assert n.daemon.is_in_stderr('bool_opt: ! does not parse as type bool') # What happens if we give it a bad int-option? @@ -93,8 +94,24 @@ def test_option_types(node_factory): }, may_fail=True, expect_fail=True) # the node should fail to start, and we get a stderr msg + assert not n.daemon.running assert n.daemon.is_in_stderr('--int_opt: notok does not parse as type int') + plugin_path = os.path.join(os.getcwd(), 'tests/plugins/options.py') + n = node_factory.get_node(options={ + 'plugin': plugin_path, + 'str_opt': 'ok', + 'int_opt': 22, + 'bool_opt': 1, + }) + + n.daemon.is_in_log(r"option str_opt ok ") + n.daemon.is_in_log(r"option int_opt 22 ") + n.daemon.is_in_log(r"option int_opt 22 ") + n.daemon.is_in_log(r"option bool_opt True ") + n.daemon.is_in_log(r"option bool_opt true ") + n.stop() + def test_millisatoshi_passthrough(node_factory): """ Ensure that Millisatoshi arguments and return work.