plugins: support concatenation of multiple args.

"multi" means that specifying a parameter twice will append, not override.
Multi args are always given as a JSON array, even if only one.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: Plugins: new "multi" field allows an option to be specified multiple times.

contrib/pyln-client/pyln/client/plugin.py

@@ -326,7 +326,8 @@ class Plugin(object):
 
     def add_option(self, name: str, default: Optional[str],
                    description: Optional[str],
-                   opt_type: str = "string", deprecated: bool = False) -> None:
+                   opt_type: str = "string", deprecated: bool = False,
+                   multi: bool = False) -> None:
         """Add an option that we'd like to register with lightningd.
 
         Needs to be called before `Plugin.run`, otherwise we might not
@@ -349,6 +350,7 @@ class Plugin(object):
             'description': description,
             'type': opt_type,
             'value': None,
+            'multi': multi,
             'deprecated': deprecated,
         }
 

doc/PLUGINS.md

@@ -157,6 +157,10 @@ There are currently four supported option 'types':
   - int: parsed as a signed integer (64-bit)
   - flag: no-arg flag option. Is boolean under the hood. Defaults to false.
 
+In addition, string and int types can specify `"multi": true` to indicate
+they can be specified multiple times.  These will always be represented in
+`init` as a (possibly empty) JSON array.
+
 Nota bene: if a `flag` type option is not set, it will not appear
 in the options set that is passed to the plugin.
 
@@ -187,6 +191,13 @@ Here's an example option set, as sent in response to `getmanifest`
       "type": "int",
       "default": 6666,
       "description": "Port to use to connect to 3rd-party service"
+    },
+    {
+      "name": "number",
+      "type": "int",
+      "default": 0,
+      "description": "Another number to add",
+	  "multi": true
     }
   ],
 ```
@@ -201,7 +212,8 @@ simple JSON object containing the options:
 ```json
 {
   "options": {
-    "greeting": "World"
+    "greeting": "World",
+	"number": [0]
   },
   "configuration": {
     "lightning-dir": "/home/user/.lightning/testnet",

lightningd/plugin.c

@@ -646,50 +646,73 @@ struct io_plan *plugin_stdout_conn_init(struct io_conn *conn,
 			       plugin_read_json, plugin);
 }
 
-char *plugin_opt_flag_set(struct plugin_opt *popt)
-{
-	/* A set flag is a true */
-	*popt->value->as_bool = true;
-	return NULL;
-}
 
-char *plugin_opt_set(const char *arg, struct plugin_opt *popt)
+/* Returns NULL if invalid value for that type */
+static struct plugin_opt_value *plugin_opt_value(const tal_t *ctx,
+						 const char *type,
+						 const char *arg)
 {
-	char *endp;
-	long long l;
+	struct plugin_opt_value *v = tal(ctx, struct plugin_opt_value);
 
-	/* Warn them that this is deprecated */
-	if (popt->deprecated && !deprecated_apis)
-		return tal_fmt(tmpctx, "deprecated option (will be removed!)");
+	v->as_str = tal_strdup(v, arg);
+	if (streq(type, "int")) {
+		long long l;
+		char *endp;
 
-	tal_free(popt->value->as_str);
-
-	popt->value->as_str = tal_strdup(popt, arg);
-	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;
+			return tal_free(v);
+		v->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")) {
+		if (v->as_int != l)
+			return tal_free(v);
+	} else if (streq(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;
+			v->as_bool = true;
 		} else if (streq(arg, "false") || streq(arg, "False")
 				|| streq(arg, "0")) {
-			*popt->value->as_bool = false;
+			v->as_bool = false;
 		} else
-			return tal_fmt(tmpctx, "%s does not parse as type %s",
-				       popt->value->as_str, popt->type);
+			return tal_free(v);
+	} else if (streq(type, "flag")) {
+		v->as_bool = true;
 	}
 
+	return v;
+}
+
+char *plugin_opt_flag_set(struct plugin_opt *popt)
+{
+	/* A set flag is a true */
+	tal_free(popt->values);
+	popt->values = tal_arr(popt, struct plugin_opt_value *, 1);
+	popt->values[0] = plugin_opt_value(popt->values, popt->type, "true");
+	return NULL;
+}
+
+char *plugin_opt_set(const char *arg, struct plugin_opt *popt)
+{
+	struct plugin_opt_value *v;
+
+	/* Warn them that this is deprecated */
+	if (popt->deprecated && !deprecated_apis)
+		return tal_fmt(tmpctx, "deprecated option (will be removed!)");
+
+	if (!popt->multi) {
+		tal_free(popt->values);
+		popt->values = tal_arr(popt, struct plugin_opt_value *, 0);
+	}
+
+	v = plugin_opt_value(popt->values, popt->type, arg);
+	if (!v)
+		return tal_fmt(tmpctx, "%s does not parse as type %s",
+			       arg, popt->type);
+	tal_arr_expand(&popt->values, v);
+
 	return NULL;
 }
 
@@ -705,13 +728,14 @@ static void destroy_plugin_opt(struct plugin_opt *opt)
 static const char *plugin_opt_add(struct plugin *plugin, const char *buffer,
 				  const jsmntok_t *opt)
 {
-	const jsmntok_t *nametok, *typetok, *defaulttok, *desctok, *deptok;
+	const jsmntok_t *nametok, *typetok, *defaulttok, *desctok, *deptok, *multitok;
 	struct plugin_opt *popt;
 	nametok = json_get_member(buffer, opt, "name");
 	typetok = json_get_member(buffer, opt, "type");
 	desctok = json_get_member(buffer, opt, "description");
 	defaulttok = json_get_member(buffer, opt, "default");
 	deptok = json_get_member(buffer, opt, "deprecated");
+	multitok = json_get_member(buffer, opt, "multi");
 
 	if (!typetok || !nametok || !desctok) {
 		return tal_fmt(plugin,
@@ -719,7 +743,7 @@ static const char *plugin_opt_add(struct plugin *plugin, const char *buffer,
 	}
 
 	popt = tal(plugin, struct plugin_opt);
-	popt->value = talz(popt, struct plugin_opt_value);
+	popt->values = tal_arr(popt, struct plugin_opt_value *, 0);
 
 	popt->name = tal_fmt(popt, "--%.*s", nametok->end - nametok->start,
 			     buffer + nametok->start);
@@ -734,45 +758,46 @@ static const char *plugin_opt_add(struct plugin *plugin, const char *buffer,
 	} else
 		popt->deprecated = false;
 
+	if (multitok) {
+		if (!json_to_bool(buffer, multitok, &popt->multi))
+			return tal_fmt(plugin,
+				       "%s: invalid \"multi\" field %.*s",
+				       popt->name,
+				       multitok->end - multitok->start,
+				       buffer + multitok->start);
+	} else
+		popt->multi = false;
+
+	popt->def = NULL;
 	if (json_tok_streq(buffer, typetok, "string")) {
 		popt->type = "string";
-		if (defaulttok) {
-			popt->value->as_str = json_strdup(popt, buffer, defaulttok);
-			popt->description = tal_fmt(
-					popt, "%.*s (default: %s)", desctok->end - desctok->start,
-					buffer + desctok->start, popt->value->as_str);
-		}
 	} else if (json_tok_streq(buffer, typetok, "int")) {
 		popt->type = "int";
-		popt->value->as_int = talz(popt->value, s64);
-		if (defaulttok) {
-			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: %"PRIu64")", desctok->end - desctok->start,
-					buffer + desctok->start, *popt->value->as_int);
-		}
-	} else if (json_tok_streq(buffer, typetok, "bool")) {
-		popt->type = "bool";
-		popt->value->as_bool = talz(popt->value, bool);
-		if (defaulttok) {
-			json_to_bool(buffer, defaulttok, popt->value->as_bool);
-			popt->value->as_str = tal_fmt(popt->value, *popt->value->as_bool ? "true" : "false");
-			popt->description = tal_fmt(
-					popt, "%.*s (default: %s)", desctok->end - desctok->start,
-					buffer + desctok->start, *popt->value->as_bool ? "true" : "false");
-		}
-	} else if (json_tok_streq(buffer, typetok, "flag")) {
-		popt->type = "flag";
-		popt->value->as_bool = talz(popt->value, bool);
+	} else if (json_tok_streq(buffer, typetok, "bool")
+		   || json_tok_streq(buffer, typetok, "flag")) {
+		popt->type = json_strdup(popt, buffer, typetok);
+		if (popt->multi)
+			return tal_fmt(plugin,
+				       "%s type \"%s\" cannot have multi",
+				       popt->name, popt->type);
 		/* We default flags to false, the default token is ignored */
-		*popt->value->as_bool = false;
-
+		if (json_tok_streq(buffer, typetok, "flag"))
+			defaulttok = NULL;
 	} else {
 		return tal_fmt(plugin,
 			       "Only \"string\", \"int\", \"bool\", and \"flag\" options are supported");
 	}
 
+	if (defaulttok) {
+		popt->def = plugin_opt_value(popt, popt->type,
+					     json_strdup(tmpctx, buffer, defaulttok));
+		if (!popt->def)
+			return tal_fmt(tmpctx, "default %.*s is not a valid %s",
+				       json_tok_full_len(defaulttok),
+				       json_tok_full(buffer, defaulttok),
+				       popt->type);
+	}
+
 	if (!popt->description)
 		popt->description = json_strdup(popt, buffer, desctok);
 
@@ -1506,6 +1531,25 @@ static void plugin_config_cb(const char *buffer,
 	check_plugins_initted(plugin->plugins);
 }
 
+static void json_add_plugin_opt(struct json_stream *stream,
+				const char *name,
+				const char *type,
+				const struct plugin_opt_value *value)
+{
+	if (streq(type, "flag")) {
+		/* We don't include 'flag' types if they're not
+		 * flagged on */
+		if (value->as_bool)
+			json_add_bool(stream, name, value->as_bool);
+	} else if (streq(type, "bool")) {
+		json_add_bool(stream, name, value->as_bool);
+	} else if (streq(type, "string")) {
+		json_add_string(stream, name, value->as_str);
+	} else if (streq(type, "int")) {
+		json_add_s64(stream, name, value->as_int);
+	}
+}
+
 void
 plugin_populate_init_request(struct plugin *plugin, struct jsonrpc_request *req)
 {
@@ -1518,17 +1562,24 @@ 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) {
-			/* We don't include 'flag' types if they're not
-			 * flagged on */
-			if (streq(opt->type, "flag") && !*opt->value->as_bool)
+
+		/* If no values, assign default (if any!) */
+		if (tal_count(opt->values) == 0) {
+			if (opt->def)
+				tal_arr_expand(&opt->values, opt->def);
+			else
 				continue;
+		}
 
-			json_add_bool(req->stream, name, *opt->value->as_bool);
-		} else if (opt->value->as_int) {
-			json_add_s64(req->stream, name, *opt->value->as_int);
-		} else if (opt->value->as_str) {
-			json_add_string(req->stream, name, opt->value->as_str);
+		if (opt->multi) {
+			json_array_start(req->stream, name);
+			for (size_t i = 0; i < tal_count(opt->values); i++)
+				json_add_plugin_opt(req->stream, NULL,
+						    opt->type, opt->values[i]);
+			json_array_end(req->stream);
+		} else {
+			json_add_plugin_opt(req->stream, name,
+					    opt->type, opt->values[0]);
 		}
 	}
 	json_object_end(req->stream); /* end of .params.options */
@@ -1626,12 +1677,19 @@ void json_add_opt_plugins_array(struct json_stream *response,
 
 				/* Trim the `--` that we added before */
 				opt_name = opt->name + 2;
-				if (opt->value->as_bool) {
-					json_add_bool(response, opt_name, *opt->value->as_bool);
-				} else if (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);
+				if (opt->multi) {
+					json_array_start(response, opt_name);
+					for (size_t i = 0; i < tal_count(opt->values); i++)
+						json_add_plugin_opt(response,
+								    NULL,
+								    opt->type,
+								    opt->values[i]);
+					json_array_end(response);
+				} else if (tal_count(opt->values)) {
+					json_add_plugin_opt(response,
+							    opt_name,
+							    opt->type,
+							    opt->values[0]);
 				} else {
 					json_add_null(response, opt_name);
 				}

lightningd/plugin.h

@@ -131,8 +131,8 @@ struct plugins {
  */
 struct plugin_opt_value {
 	char *as_str;
-	s64 *as_int;
-	bool *as_bool;
+	s64 as_int;
+	bool as_bool;
 };
 
 /**
@@ -144,7 +144,10 @@ struct plugin_opt {
 	const char *name;
 	const char *type;
 	const char *description;
-	struct plugin_opt_value *value;
+	struct plugin_opt_value **values;
+	/* Might be NULL if no default */
+	struct plugin_opt_value *def;
+	bool multi;
 	bool deprecated;
 };
 

tests/plugins/options.py

@@ -18,4 +18,8 @@ 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.add_flag_option('flag_opt', 'an example flag type option')
+
+plugin.add_option('str_optm', None, 'an example string option', multi=True)
+plugin.add_option('int_optm', 7, 'an example int type option', opt_type='int', multi=True)
+
 plugin.run()

tests/test_plugin.py

@@ -120,6 +120,16 @@ def test_option_types(node_factory):
     assert not n.daemon.running
     assert n.daemon.is_in_stderr("--flag_opt: doesn't allow an argument")
 
+    n = node_factory.get_node(options={
+        'plugin': plugin_path,
+        'str_optm': ['ok', 'ok2'],
+        'int_optm': [11, 12, 13],
+    })
+
+    assert n.daemon.is_in_log(r"option str_optm \['ok', 'ok2'\] <class 'list'>")
+    assert n.daemon.is_in_log(r"option int_optm \[11, 12, 13\] <class 'list'>")
+    n.stop()
+
 
 def test_millisatoshi_passthrough(node_factory):
     """ Ensure that Millisatoshi arguments and return work.