Pass typed plugin option data to plugin in init

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,

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.
@@ -973,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()
@@ -1004,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

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;
 }
 
@@ -1102,6 +1126,16 @@ 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);
+			if (!deprecated_apis)
+				continue;
+		}
+		if (opt->value->as_int) {
+			json_add_s64(req->stream, name, *opt->value->as_int);
+			if (!deprecated_apis)
+				continue;
+		}
 		if (opt->value->as_str) {
 			json_add_string(req->stream, name, opt->value->as_str);
 		}
@@ -1177,7 +1211,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 {

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;
 };
 

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()

tests/test_plugin.py

@@ -46,6 +46,73 @@ 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': True,
+    })
+
+    n.daemon.is_in_log(r"option str_opt ok <class 'str'>")
+    n.daemon.is_in_log(r"option int_opt 22 <class 'int'>")
+    n.daemon.is_in_log(r"option bool_opt True <class 'bool'>")
+    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 <class 'bool'>")
+    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 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?
+    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 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 <class 'str'>")
+    n.daemon.is_in_log(r"option int_opt 22 <class 'int'>")
+    n.daemon.is_in_log(r"option int_opt 22 <class 'str'>")
+    n.daemon.is_in_log(r"option bool_opt True <class 'bool'>")
+    n.daemon.is_in_log(r"option bool_opt true <class 'str'>")
+    n.stop()
+
+
 def test_millisatoshi_passthrough(node_factory):
     """ Ensure that Millisatoshi arguments and return work.
     """