lightningd: accept options array in plugin start

contrib/msggen/msggen/schema.json

@@ -32987,7 +32987,7 @@
             ],
             "description": [
               "Determines what action is taken:",
-              "  - *subcommand* **start** takes a *path* to an executable as argument and starts it as plugin. *path* may be an absolute path or a path relative to the plugins directory (default *~/.lightning/plugins*). If the plugin is already running and the executable (checksum) has changed, the plugin is killed and restarted except if its an important (or builtin) plugin. If the plugin doesn't complete the 'getmanifest' and 'init' handshakes within 60 seconds, the command will timeout and kill the plugin. Additional *options* may be passed to the plugin, but requires all parameters to be passed as keyword=value pairs using the `-k|--keyword` option which is recommended. For example the following command starts the plugin helloworld.py (present in the plugin directory) with the option greeting set to 'A crazy':",
+              "  - *subcommand* **start** takes a *path* to an executable as argument and starts it as plugin. *path* may be an absolute path or a path relative to the plugins directory (default *~/.lightning/plugins*). If the plugin is already running and the executable (checksum) has changed, the plugin is killed and restarted except if its an important (or builtin) plugin. If the plugin doesn't complete the 'getmanifest' and 'init' handshakes within 60 seconds, the command will timeout and kill the plugin. Additional *options* may be passed to the plugin, either as an *options* array of *keyword=value* strings, or as flattened keyword=value parameters using the `-k|--keyword` option which is recommended. For example the following command starts the plugin helloworld.py (present in the plugin directory) with the option greeting set to 'A crazy':",
               "    ```shell",
               "    lightning-cli -k plugin subcommand=start plugin=helloworld.py greeting='A crazy'",
               "    ```",

doc/schemas/plugin.json

@@ -30,7 +30,7 @@
         ],
         "description": [
           "Determines what action is taken:",
-          "  - *subcommand* **start** takes a *path* to an executable as argument and starts it as plugin. *path* may be an absolute path or a path relative to the plugins directory (default *~/.lightning/plugins*). If the plugin is already running and the executable (checksum) has changed, the plugin is killed and restarted except if its an important (or builtin) plugin. If the plugin doesn't complete the 'getmanifest' and 'init' handshakes within 60 seconds, the command will timeout and kill the plugin. Additional *options* may be passed to the plugin, but requires all parameters to be passed as keyword=value pairs using the `-k|--keyword` option which is recommended. For example the following command starts the plugin helloworld.py (present in the plugin directory) with the option greeting set to 'A crazy':",
+          "  - *subcommand* **start** takes a *path* to an executable as argument and starts it as plugin. *path* may be an absolute path or a path relative to the plugins directory (default *~/.lightning/plugins*). If the plugin is already running and the executable (checksum) has changed, the plugin is killed and restarted except if its an important (or builtin) plugin. If the plugin doesn't complete the 'getmanifest' and 'init' handshakes within 60 seconds, the command will timeout and kill the plugin. Additional *options* may be passed to the plugin, either as an *options* array of *keyword=value* strings, or as flattened keyword=value parameters using the `-k|--keyword` option which is recommended. For example the following command starts the plugin helloworld.py (present in the plugin directory) with the option greeting set to 'A crazy':",
           "    ```shell",
           "    lightning-cli -k plugin subcommand=start plugin=helloworld.py greeting='A crazy'",
           "    ```",

lightningd/plugin_control.c

@@ -1,4 +1,5 @@
 #include "config.h"
+#include <ccan/json_escape/json_escape.h>
 #include <ccan/tal/path/path.h>
 #include <ccan/tal/str/str.h>
 #include <common/json_command.h>
@@ -86,6 +87,59 @@ plugin_dynamic_start(struct plugin_command *pcmd, const char *plugin_path,
 	return command_still_pending(pcmd->cmd);
 }
 
+/* Returns true if params is an object with keys other than the fixed
+ * subcommand/plugin/options. */
+static bool plugin_has_extra_params(const char *buffer,
+				    const jsmntok_t *params)
+{
+	size_t i;
+	const jsmntok_t *t;
+
+	json_for_each_obj(i, t, params)
+		if (!json_tok_streq(buffer, t, "subcommand")
+		    && !json_tok_streq(buffer, t, "plugin")
+		    && !json_tok_streq(buffer, t, "options"))
+			return true;
+	return false;
+}
+
+/* Build an object of name/value pairs from an "options" array of
+ * "keyword=value" strings, suitable for plugin_add_params().  An element
+ * without an '=' is treated as a boolean flag. */
+static jsmntok_t *plugin_start_params(const tal_t *ctx, const char *buffer,
+				      const jsmntok_t *options, char **parambuf)
+{
+	size_t i;
+	const jsmntok_t *t;
+	char *newbuf = tal_strdup(ctx, "{");
+	bool first = true;
+
+	json_for_each_arr(i, t, options) {
+		const char *opt = json_strdup(tmpctx, buffer, t);
+		const char *eq = strchr(opt, '=');
+		struct json_escape *esc;
+
+		if (!first)
+			tal_append_fmt(&newbuf, ",");
+		first = false;
+		if (eq) {
+			esc = json_escape(tmpctx,
+					  take(tal_strndup(tmpctx, opt, eq - opt)));
+			tal_append_fmt(&newbuf, "\"%s\":", esc->s);
+			esc = json_escape(tmpctx, eq + 1);
+			tal_append_fmt(&newbuf, "\"%s\"", esc->s);
+		} else {
+			/* Boolean flags are given without a value. */
+			esc = json_escape(tmpctx, opt);
+			tal_append_fmt(&newbuf, "\"%s\":true", esc->s);
+		}
+	}
+
+	tal_append_fmt(&newbuf, "}");
+	*parambuf = newbuf;
+	return json_parse_simple(ctx, newbuf, strlen(newbuf));
+}
+
 /**
  * Called when trying to start a plugin directory through RPC, it registers
  * all contained plugins recursively and then starts them.
@@ -235,23 +289,54 @@ static struct command_result *json_plugin_control(struct command *cmd,
 		return plugin_dynamic_stop(cmd, plugin_name);
 	} else if (streq(subcmd, "start")) {
 		const char *plugin_path;
+		const jsmntok_t *options = NULL;
+		char *mod_buffer;
 		jsmntok_t *mod_params;
 
 		if (!param_check(cmd, buffer, params,
 				 p_req("subcommand", param_ignore, cmd),
 				 p_req("plugin", param_string, &plugin_path),
+				 p_opt("options", param_array, &options),
 				 p_opt_any(),
 				 NULL))
 			return command_param_failed();
 
+		/* The "options" array is documented as containing keyword=value
+		 * strings: reject anything else before we mangle it. */
+		if (options) {
+			size_t i;
+			const jsmntok_t *t;
+
+			json_for_each_arr(i, t, options)
+				if (t->type != JSMN_STRING)
+					return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+							    "options array entries must be strings");
+		}
+
 		/* Manually parse any remaining options (only for objects,
 		 * since plugin options must be explicitly named!). */
 		if (params->type == JSMN_ARRAY) {
-			if (params->size != 2)
+			if (params->size > (options ? 3 : 2))
 				return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
 						    "Extra parameters must be in object");
-			mod_params = NULL;
+			if (options) {
+				mod_params = plugin_start_params(cmd, buffer,
+								options, &mod_buffer);
+			} else {
+				mod_buffer = NULL;
+				mod_params = NULL;
+			}
+		} else if (options) {
+			/* Either flattened keyword options or an explicit
+			 * "options" array, never both. */
+			if (plugin_has_extra_params(buffer, params))
+				return command_fail(cmd, JSONRPC2_INVALID_PARAMS,
+						    "Cannot mix 'options' array "
+						    "with keyword options");
+			mod_params = plugin_start_params(cmd, buffer,
+							options, &mod_buffer);
 		} else {
+			mod_buffer = NULL;
 			mod_params = json_tok_copy(cmd, params);
 
 			json_tok_remove(&mod_params, mod_params,
@@ -271,7 +356,9 @@ static struct command_result *json_plugin_control(struct command *cmd,
 		if (command_check_only(cmd))
 			return command_check_done(cmd);
 
-		return plugin_dynamic_start(pcmd, plugin_path, buffer, mod_params);
+		return plugin_dynamic_start(pcmd, plugin_path,
+					    mod_buffer ? mod_buffer : buffer,
+					    mod_params);
 	} else if (streq(subcmd, "startdir")) {
 		const char *dir_path;
 

tests/plugins/dynamic_option.py

@@ -5,16 +5,20 @@
 plugin = Plugin()
 
 
-@plugin.method('dynamic-option-report')
+@plugin.method("dynamic-option-report")
 def record_lookup(plugin):
-    return {'test-dynamic-config': plugin.get_option('test-dynamic-config')}
+    return {
+        "test-dynamic-config": plugin.get_option("test-dynamic-config"),
+        "test-dynamic-int": plugin.get_option("test-dynamic-int"),
+        "test-dynamic-bool": plugin.get_option("test-dynamic-bool"),
+        "test-dynamic-flag": plugin.get_option("test-dynamic-flag"),
+    }
 
 
 def on_config_change(plugin, config: str, value: Optional[Any]) -> None:
-    """Callback method called when a config value is changed.
-    """
+    """Callback method called when a config value is changed."""
     plugin.log(f"Setting config {config} to {value}")
-    if value == 'bad value':
+    if value == "bad value":
         raise RpcException("I don't like bad values!")
 
 
@@ -26,5 +30,27 @@ def on_config_change(plugin, config: str, value: Optional[Any]) -> None:
     on_change=on_config_change,
 )
 
+plugin.add_option(
+    name="test-dynamic-int",
+    description="An int option which can be changed at run-time",
+    default=0,
+    opt_type="int",
+    dynamic=True,
+)
+
+plugin.add_option(
+    name="test-dynamic-bool",
+    description="A bool option which can be changed at run-time",
+    default=False,
+    opt_type="bool",
+    dynamic=True,
+)
+
+plugin.add_flag_option(
+    name="test-dynamic-flag",
+    description="A flag option which can be changed at run-time",
+    dynamic=True,
+)
+
 
 plugin.run()

tests/test_plugin.py

@@ -3060,6 +3060,108 @@ def test_dynamic_args(node_factory):
     assert 'greeting' not in l1.rpc.listconfigs()['configs']
 
 
+def test_dynamic_args_options_array(node_factory):
+    """plugin start accepts options as an explicit array, matching the schema."""
+    plugin_path = os.path.join(os.getcwd(), "tests/plugins/dynamic_option.py")
+
+    l1 = node_factory.get_node()
+
+    # Positionally, an options array is a valid 3rd argument, covering every
+    # option type: string, int, bool and flag (which has no '=').
+    l1.rpc.call(
+        "plugin",
+        [
+            "start",
+            plugin_path,
+            [
+                "test-dynamic-config=Test options array",
+                "test-dynamic-int=42",
+                "test-dynamic-bool=false",
+                "test-dynamic-flag",
+            ],
+        ],
+    )
+    assert l1.rpc.dynamic_option_report() == {
+        "test-dynamic-config": "Test options array",
+        "test-dynamic-int": 42,
+        "test-dynamic-bool": False,
+        "test-dynamic-flag": True,
+    }
+    l1.rpc.plugin_stop(plugin_path)
+
+    l1.rpc.call(
+        "plugin",
+        {
+            "subcommand": "start",
+            "plugin": plugin_path,
+            "options": [
+                "test-dynamic-config=Test options array",
+                "test-dynamic-int=42",
+                "test-dynamic-bool=false",
+                "test-dynamic-flag",
+            ],
+        },
+    )
+    assert l1.rpc.dynamic_option_report() == {
+        "test-dynamic-config": "Test options array",
+        "test-dynamic-int": 42,
+        "test-dynamic-bool": False,
+        "test-dynamic-flag": True,
+    }
+    l1.rpc.plugin_stop(plugin_path)
+
+    l1.rpc.call(
+        "plugin",
+        {
+            "subcommand": "start",
+            "plugin": plugin_path,
+            "test-dynamic-config": "Test options array",
+            "test-dynamic-int": 42,
+            "test-dynamic-bool": False,
+            "test-dynamic-flag": None,
+        },
+    )
+    assert l1.rpc.dynamic_option_report() == {
+        "test-dynamic-config": "Test options array",
+        "test-dynamic-int": 42,
+        "test-dynamic-bool": False,
+        "test-dynamic-flag": None,
+    }
+    l1.rpc.plugin_stop(plugin_path)
+
+    # Entries must be strings, not e.g. numbers.
+    with pytest.raises(RpcError, match="options array entries must be strings"):
+        l1.rpc.call("plugin", ["start", plugin_path, [42]])
+
+    # ...but any trailing positional args are ambiguous, so must be rejected.
+    with pytest.raises(RpcError, match="Extra parameters must be in object"):
+        l1.rpc.call(
+            "plugin",
+            [
+                "start",
+                plugin_path,
+                ["test-dynamic-config=Test options array"],
+                "test-dynamic-config=yikes",
+            ],
+        )
+
+    # The same applies if there is no options array at all.
+    with pytest.raises(RpcError, match="Extra parameters must be in object"):
+        l1.rpc.call("plugin", ["start", plugin_path, None, "test-dynamic-config=yikes"])
+
+    # ...and flattened keyword options cannot be mixed with an options array.
+    with pytest.raises(RpcError, match="Cannot mix"):
+        l1.rpc.call(
+            "plugin",
+            {
+                "subcommand": "start",
+                "plugin": plugin_path,
+                "options": ["test-dynamic-config=Test options array"],
+                "test-dynamic-config": "yikes",
+            },
+        )
+
+
 def test_pyln_request_notify(node_factory):
     """Test that pyln-client plugins can send notifications.
     """
@@ -4654,10 +4756,10 @@ def test_dynamic_option_python_plugin(node_factory):
 
     assert result["configs"]["test-dynamic-config"]["value_str"] == "initial"
 
-    assert l1.rpc.dynamic_option_report() == {'test-dynamic-config': 'initial'}
+    assert l1.rpc.dynamic_option_report()['test-dynamic-config'] == 'initial'
     result = l1.rpc.setconfig("test-dynamic-config", "changed")
     assert result["config"]["value_str"] == "changed"
-    assert l1.rpc.dynamic_option_report() == {'test-dynamic-config': 'changed'}
+    assert l1.rpc.dynamic_option_report()['test-dynamic-config'] == 'changed'
 
     l1.daemon.wait_for_log(
         'dynamic_option.py:.*Setting config test-dynamic-config to changed'
@@ -4667,7 +4769,7 @@ def test_dynamic_option_python_plugin(node_factory):
         l1.rpc.setconfig("test-dynamic-config", "bad value")
 
     # Does not alter value!
-    assert l1.rpc.dynamic_option_report() == {'test-dynamic-config': 'changed'}
+    assert l1.rpc.dynamic_option_report()['test-dynamic-config'] == 'changed'
 
 
 def test_renepay_not_important(node_factory):