From 278ea5e16da8c0551e57f0520be327128ff35a89 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Wed, 29 Jul 2020 16:20:49 +0800 Subject: [PATCH 1/7] lightningd/plugin.c: Add specific function to give the directory for built-in plugins. --- lightningd/lightningd.c | 15 ++++++++------- lightningd/plugin.c | 11 +++++++++++ lightningd/plugin.h | 6 ++++++ lightningd/test/run-find_my_abspath.c | 3 +++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 397ce48d66e3..fc8a80714aae 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -469,9 +469,9 @@ static const char *find_my_pkglibexec_path(struct lightningd *ld, /*~ The plugin dir is in ../libexec/c-lightning/plugins, which (unlike * those given on the command line) does not need to exist. */ - add_plugin_dir(ld->plugins, - path_join(tmpctx, pkglibexecdir, "plugins"), - true); + plugins_set_builtin_plugins_dir(ld->plugins, + path_join(tmpctx, + pkglibexecdir, "plugins")); /*~ Sometimes take() can be more efficient, since the routine can * manipulate the string in place. This is the case here. */ @@ -484,10 +484,11 @@ static const char *find_daemon_dir(struct lightningd *ld, const char *argv0) const char *my_path = find_my_directory(ld, argv0); /* If we're running in-tree, all the subdaemons are with lightningd. */ if (has_all_subdaemons(my_path)) { - /* In this case, look in ../plugins */ - add_plugin_dir(ld->plugins, - path_join(tmpctx, my_path, "../plugins"), - true); + /* In this case, look for built-in plugins in ../plugins */ + plugins_set_builtin_plugins_dir(ld->plugins, + path_join(tmpctx, + my_path, + "../plugins")); return my_path; } diff --git a/lightningd/plugin.c b/lightningd/plugin.c index b21a127f1e5a..dc8b577d149a 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -1526,6 +1526,16 @@ struct log *plugin_get_log(struct plugin *plugin) return plugin->log; } +void plugins_set_builtin_plugins_dir(struct plugins *plugins, + const char *dir) +{ + /*~ The builtin-plugins dir does not need to exist, but + * we would error those anyway for our important built-in + * plugins. + */ + add_plugin_dir(plugins, dir, true); +} + struct plugin_destroyed { const struct plugin *plugin; }; @@ -1536,6 +1546,7 @@ static void mark_plugin_destroyed(const struct plugin *unused, pd->plugin = NULL; } + struct plugin_destroyed *plugin_detect_destruction(const struct plugin *plugin) { struct plugin_destroyed *pd = tal(NULL, struct plugin_destroyed); diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 0b1442ceca86..5dc98346f3cc 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -334,6 +334,12 @@ struct io_plan *plugin_stdout_conn_init(struct io_conn *conn, */ struct log *plugin_get_log(struct plugin *plugin); +/** + * Tells the plugin system the directory for builtin plugins. + */ +void plugins_set_builtin_plugins_dir(struct plugins *plugins, + const char *dir); + /* Pair of functions to detect if plugin destroys itself: must always * call both! */ struct plugin_destroyed *plugin_detect_destruction(const struct plugin *plugin); diff --git a/lightningd/test/run-find_my_abspath.c b/lightningd/test/run-find_my_abspath.c index f62e15697bd1..1e9a73fe9bdc 100644 --- a/lightningd/test/run-find_my_abspath.c +++ b/lightningd/test/run-find_my_abspath.c @@ -207,6 +207,9 @@ void plugins_init(struct plugins *plugins UNNEEDED) struct plugins *plugins_new(const tal_t *ctx UNNEEDED, struct log_book *log_book UNNEEDED, struct lightningd *ld UNNEEDED) { fprintf(stderr, "plugins_new called!\n"); abort(); } +void plugins_set_builtin_plugins_dir(struct plugins *plugins UNNEEDED, + const char *dir UNNEEDED) +{ fprintf(stderr, "plugins_set_builtin_plugins_dir called!\n"); abort(); } /* Generated stub for setup_color_and_alias */ void setup_color_and_alias(struct lightningd *ld UNNEEDED) { fprintf(stderr, "setup_color_and_alias called!\n"); abort(); } From 340d66ee592a3e90d9033a0908b8c68f597e3d68 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Wed, 29 Jul 2020 16:56:42 +0800 Subject: [PATCH 2/7] lightningd/lightningd.c: Create API to exit lightningd with an exit code. --- lightningd/lightningd.c | 42 ++++++++++++++++++++++++++++++++--------- lightningd/lightningd.h | 10 ++++++++++ 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index fc8a80714aae..c7cbedd2b381 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -281,6 +281,11 @@ static struct lightningd *new_lightningd(const tal_t *ctx) */ ld->rpc_filemode = 0600; + /*~ This is the exit code to use on exit. + * Set to NULL meaning we are not interested in exiting yet. + */ + ld->exit_code = NULL; + return ld; } @@ -752,6 +757,14 @@ static void hsm_ecdh_failed(enum status_failreason fail, fatal("hsm failure: %s", fmt); } +/*~ This signals to the mainloop that some part wants to cleanly exit now. */ +void lightningd_exit(struct lightningd *ld, int exit_code) +{ + ld->exit_code = tal(ld, int); + *ld->exit_code = exit_code; + io_break(ld); +} + int main(int argc, char *argv[]) { struct lightningd *ld; @@ -764,6 +777,8 @@ int main(int argc, char *argv[]) struct ext_key *bip32_base; struct rlimit nofile = {1024, 1024}; + int exit_code = 0; + /*~ Make sure that we limit ourselves to something reasonable. Modesty * is a virtue. */ setrlimit(RLIMIT_NOFILE, &nofile); @@ -1003,10 +1018,17 @@ int main(int argc, char *argv[]) assert(io_loop_ret == ld); ld->state = LD_STATE_SHUTDOWN; - /* Keep this fd around, to write final response at the end. */ - stop_fd = io_conn_fd(ld->stop_conn); - io_close_taken_fd(ld->stop_conn); - stop_response = tal_steal(NULL, ld->stop_response); + /* Were we exited via `lightningd_exit`? */ + if (ld->exit_code) { + exit_code = *ld->exit_code; + stop_fd = -1; + stop_response = NULL; + } else { + /* Keep this fd around, to write final response at the end. */ + stop_fd = io_conn_fd(ld->stop_conn); + io_close_taken_fd(ld->stop_conn); + stop_response = tal_steal(NULL, ld->stop_response); + } shutdown_subdaemons(ld); @@ -1041,11 +1063,13 @@ int main(int argc, char *argv[]) daemon_shutdown(); - /* Finally, send response to shutdown command */ - write_all(stop_fd, stop_response, strlen(stop_response)); - close(stop_fd); - tal_free(stop_response); + /* Finally, send response to shutdown command if appropriate. */ + if (stop_fd >= 0) { + write_all(stop_fd, stop_response, strlen(stop_response)); + close(stop_fd); + tal_free(stop_response); + } /*~ Farewell. Next stop: hsmd/hsmd.c. */ - return 0; + return exit_code; } diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index b5d7b3698b9e..5ad15243ee6c 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -271,6 +271,9 @@ struct lightningd { /* Total number of coin moves we've seen, since * coin move tracking was cool */ s64 coin_moves_count; + + /* If non-NULL, contains the exit code to use. */ + int *exit_code; }; /* Turning this on allows a tal allocation to return NULL, rather than aborting. @@ -289,4 +292,11 @@ void test_subdaemons(const struct lightningd *ld); /* Notify lightningd about new blocks. */ void notify_new_block(struct lightningd *ld, u32 block_height); +/* Signal a clean exit from lightningd. + * NOTE! This function **returns**. + * This just causes the main loop to exit, so you have to return + * all the way to the main loop for `lightningd` to exit. + */ +void lightningd_exit(struct lightningd *ld, int exit_code); + #endif /* LIGHTNING_LIGHTNINGD_LIGHTNINGD_H */ From 6c853605c78ca7ce443f5341ddb5a24ce7af9cd7 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Wed, 29 Jul 2020 19:24:07 +0800 Subject: [PATCH 3/7] lightningd/plugin.c: Add important plugins, which if they terminate, lightningd also terminates. Changelog-Added: New option `--important-plugin` loads a plugin is so important that if it dies, `lightningd` will exit rather than continue. You can still `--disable-plugin` it, however, which trumps `--important-plugin` and it will not be started at all. --- doc/lightningd-config.5 | 17 ++++++-- doc/lightningd-config.5.md | 16 ++++++-- lightningd/options.c | 19 ++++++++- lightningd/plugin.c | 80 ++++++++++++++++++++++++++++++------- lightningd/plugin.h | 11 ++++- lightningd/plugin_control.c | 2 +- tests/test_misc.py | 2 +- 7 files changed, 123 insertions(+), 24 deletions(-) diff --git a/doc/lightningd-config.5 b/doc/lightningd-config.5 index cb18a1b51caa..5f2a6d58b73b 100644 --- a/doc/lightningd-config.5 +++ b/doc/lightningd-config.5 @@ -534,10 +534,11 @@ add multiple directories\. \fBclear-plugins\fR -This option clears all \fIplugin\fR and \fIplugin-dir\fR options preceeding it, +This option clears all \fIplugin\fR, \fIimportant-plugin\fR, and \fIplugin-dir\fR options +preceeding it, including the default built-in plugin directory\. You can still add -\fIplugin-dir\fR and \fIplugin\fR options following this and they will have the -normal effect\. +\fIplugin-dir\fR, \fIplugin\fR, and \fIimportant-plugin\fR options following this +and they will have the normal effect\. \fBdisable-plugin\fR=\fIPLUGIN\fR @@ -547,6 +548,16 @@ be loaded at startup, whatever directory it is in\. This option is useful for disabling a single plugin inside a directory\. You can still explicitly load plugins which have been disabled, using \fBlightning-plugin\fR(7) \fBstart\fR\. + + \fBimportant-plugin\fR=\fIPLUGIN\fR +Speciy a plugin to run as part of C-lightning\. +This can be specified multiple times to add multiple plugins\. +Plugins specified via this option are considered so important, that if the +plugin stops for any reason (including via \fBlightning-plugin\fR(7) \fBstop\fR), +C-lightning will also stop running\. +This way, you can monitor crashes of important plugins by simply monitoring +if C-lightning terminates\. + .SH BUGS You should report bugs on our github issues page, and maybe submit a fix diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index 39122ec9df79..316a7ac152a3 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -439,10 +439,11 @@ loaded. *DIRECTORY* must exist; this can be specified multiple times to add multiple directories. **clear-plugins** -This option clears all *plugin* and *plugin-dir* options preceeding it, +This option clears all *plugin*, *important-plugin*, and *plugin-dir* options +preceeding it, including the default built-in plugin directory. You can still add -*plugin-dir* and *plugin* options following this and they will have the -normal effect. +*plugin-dir*, *plugin*, and *important-plugin* options following this +and they will have the normal effect. **disable-plugin**=*PLUGIN* If *PLUGIN* contains a /, plugins with the same path as *PLUGIN* will @@ -451,6 +452,15 @@ be loaded at startup, whatever directory it is in. This option is useful for disabling a single plugin inside a directory. You can still explicitly load plugins which have been disabled, using lightning-plugin(7) `start`. + **important-plugin**=*PLUGIN* +Speciy a plugin to run as part of C-lightning. +This can be specified multiple times to add multiple plugins. +Plugins specified via this option are considered so important, that if the +plugin stops for any reason (including via lightning-plugin(7) `stop`), +C-lightning will also stop running. +This way, you can monitor crashes of important plugins by simply monitoring +if C-lightning terminates. + BUGS ---- diff --git a/lightningd/options.c b/lightningd/options.c index e67bc73117d0..ba0099dd7b17 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -348,7 +348,7 @@ static char *opt_add_plugin(const char *arg, struct lightningd *ld) log_info(ld->log, "%s: disabled via disable-plugin", arg); return NULL; } - plugin_register(ld->plugins, arg, NULL); + plugin_register(ld->plugins, arg, NULL, false); return NULL; } @@ -369,6 +369,16 @@ static char *opt_clear_plugins(struct lightningd *ld) return NULL; } +static char *opt_important_plugin(const char *arg, struct lightningd *ld) +{ + if (plugin_blacklisted(ld->plugins, arg)) { + log_info(ld->log, "%s: disabled via disable-plugin", arg); + return NULL; + } + plugin_register(ld->plugins, arg, NULL, true); + return NULL; +} + /* Prompt the user to enter a password, from which will be derived the key used * for `hsm_secret` encryption. * The algorithm used to derive the key is Argon2(id), to which libsodium @@ -771,6 +781,10 @@ static void register_opts(struct lightningd *ld) NULL, ld, "Disable a particular plugin by filename/name"); + opt_register_early_arg("--important-plugin", opt_important_plugin, + NULL, ld, + "Add an important plugin to be run (can be used multiple times). Die if the plugin dies."); + /* Early, as it suppresses DNS lookups from cmdline too. */ opt_register_early_arg("--always-use-proxy", opt_set_bool_arg, opt_show_bool, @@ -1278,6 +1292,9 @@ static void add_config(struct lightningd *ld, json_add_opt_log_levels(response, ld->log); } else if (opt->cb_arg == (void *)opt_disable_plugin) { json_add_opt_disable_plugins(response, ld->plugins); + } else if (opt->cb_arg == (void *)opt_important_plugin) { + /* Do nothing, this is already handled by + * opt_add_plugin. */ } else if (opt->cb_arg == (void *)opt_add_plugin_dir || opt->cb_arg == (void *)plugin_opt_set || opt->cb_arg == (void *)plugin_opt_flag_set) { diff --git a/lightningd/plugin.c b/lightningd/plugin.c index dc8b577d149a..5fc4f6454fd2 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -53,6 +53,7 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, p->startup = true; p->json_cmds = tal_arr(p, struct command *, 0); p->blacklist = tal_arr(p, const char *, 0); + p->shutdown = false; uintmap_init(&p->pending_requests); memleak_add_helper(p, memleak_help_pending_requests); @@ -62,6 +63,9 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, void plugins_free(struct plugins *plugins) { struct plugin *p; + + plugins->shutdown = true; + /* Plugins are usually the unit of allocation, and they are internally * consistent, so let's free each plugin first. */ while (!list_empty(&plugins->plugins)) { @@ -105,6 +109,7 @@ struct command_result *plugin_register_all_complete(struct lightningd *ld, static void destroy_plugin(struct plugin *p) { struct plugin_rpccall *call; + plugin_hook_unregister_all(p); list_del(&p->list); @@ -118,10 +123,23 @@ static void destroy_plugin(struct plugin *p) /* Don't call this if we're still parsing options! */ if (p->plugin_state != UNCONFIGURED) check_plugins_resolved(p->plugins); + + /* If we are shutting down, do not continue to checking if + * the dying plugin is important. */ + if (p->plugins->shutdown) + return; + + /* Now check if the dying plugin is important. */ + if (p->important) { + log_broken(p->log, + "Plugin marked as important, " + "shutting down lightningd!"); + lightningd_exit(p->plugins->ld, 1); + } } struct plugin *plugin_register(struct plugins *plugins, const char* path TAKES, - struct command *start_cmd) + struct command *start_cmd, bool important) { struct plugin *p, *p_temp; @@ -130,6 +148,9 @@ struct plugin *plugin_register(struct plugins *plugins, const char* path TAKES, if (streq(path, p_temp->cmd)) { if (taken(path)) tal_free(path); + /* If added as "important", upgrade to "important". */ + if (important) + p_temp->important = true; return NULL; } } @@ -153,6 +174,8 @@ struct plugin *plugin_register(struct plugins *plugins, const char* path TAKES, list_add_tail(&plugins->plugins, &p->list); tal_add_destructor(p, destroy_plugin); list_head_init(&p->pending_rpccalls); + + p->important = important; return p; } @@ -186,6 +209,8 @@ void plugin_blacklist(struct plugins *plugins, const char *name) log_info(plugins->log, "%s: disabled via disable-plugin", p->cmd); list_del_from(&plugins->plugins, &p->list); + /* disable-plugin overrides important-plugin. */ + p->important = false; tal_free(p); } } @@ -1188,7 +1213,7 @@ char *add_plugin_dir(struct plugins *plugins, const char *dir, bool error_ok) log_info(plugins->log, "%s: disabled via disable-plugin", fullpath); } else { - p = plugin_register(plugins, fullpath, NULL); + p = plugin_register(plugins, fullpath, NULL, false); if (!p && !error_ok) return tal_fmt(NULL, "Failed to register %s: %s", fullpath, strerror(errno)); @@ -1396,24 +1421,35 @@ void plugins_config(struct plugins *plugins) plugins->startup = false; } -void json_add_opt_plugins(struct json_stream *response, - const struct plugins *plugins) +/** json_add_opt_plugins_array + * + * @brief add a named array of plugins to the given response, + * depending on whether it is important or not important. + * + * @param response - the `json_stream` to write into. + * @param name - the field name of the array. + * @param plugins - the plugins object to query. + * @param important - match the `important` setting of the + * plugins to be added. + */ +static +void json_add_opt_plugins_array(struct json_stream *response, + const char *name, + const struct plugins *plugins, + bool important) { struct plugin *p; - struct plugin_opt *opt; const char *plugin_name; + struct plugin_opt *opt; const char *opt_name; - /* DEPRECATED: duplicated JSON "plugin" entries */ - if (deprecated_apis) { - list_for_each(&plugins->plugins, p, list) { - json_add_string(response, "plugin", p->cmd); - } - } - /* we output 'plugins' and their options as an array of substructures */ - json_array_start(response, "plugins"); + json_array_start(response, name); list_for_each(&plugins->plugins, p, list) { + /* Skip if not matching. */ + if (p->important != important) + continue; + json_object_start(response, NULL); json_add_string(response, "path", p->cmd); @@ -1444,6 +1480,23 @@ void json_add_opt_plugins(struct json_stream *response, json_array_end(response); } +void json_add_opt_plugins(struct json_stream *response, + const struct plugins *plugins) +{ + struct plugin *p; + + json_add_opt_plugins_array(response, "plugins", plugins, false); + json_add_opt_plugins_array(response, "important-plugins", plugins, true); + + /* DEPRECATED: duplicated JSON "plugin" entries */ + if (deprecated_apis) { + list_for_each(&plugins->plugins, p, list) { + json_add_string(response, p->important ? "important-plugin" : "plugin", p->cmd); + } + } + +} + void json_add_opt_disable_plugins(struct json_stream *response, const struct plugins *plugins) { @@ -1546,7 +1599,6 @@ static void mark_plugin_destroyed(const struct plugin *unused, pd->plugin = NULL; } - struct plugin_destroyed *plugin_detect_destruction(const struct plugin *plugin) { struct plugin_destroyed *pd = tal(NULL, struct plugin_destroyed); diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 5dc98346f3cc..5b494746055a 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -79,6 +79,10 @@ struct plugin { /* An array of currently pending RPC method calls, to be killed if the * plugin exits. */ struct list_head pending_rpccalls; + + /* If set, the plugin is so important that if it terminates early, + * C-lightning should terminate as well. */ + bool important; }; /** @@ -103,6 +107,9 @@ struct plugins { /* Blacklist of plugins from --disable-plugin */ const char **blacklist; + + /* Whether we are shutting down (`plugins_free` is called) */ + bool shutdown; }; /* The value of a plugin option, which can have different types. @@ -175,13 +182,15 @@ void plugins_free(struct plugins *plugins); * @param plugins: Plugin context * @param path: The path of the executable for this plugin * @param start_cmd: The optional JSON command which caused this. + * @param important: The plugin is important. * * If @start_cmd, then plugin_cmd_killed or plugin_cmd_succeeded will be called * on it eventually. */ struct plugin *plugin_register(struct plugins *plugins, const char* path TAKES, - struct command *start_cmd); + struct command *start_cmd, + bool important); /** * Returns true if the provided name matches a plugin command diff --git a/lightningd/plugin_control.c b/lightningd/plugin_control.c index 87ad14bf90db..047122a05e66 100644 --- a/lightningd/plugin_control.c +++ b/lightningd/plugin_control.c @@ -58,7 +58,7 @@ struct command_result *plugin_cmd_all_complete(struct plugins *plugins, static struct command_result * plugin_dynamic_start(struct command *cmd, const char *plugin_path) { - struct plugin *p = plugin_register(cmd->ld->plugins, plugin_path, cmd); + struct plugin *p = plugin_register(cmd->ld->plugins, plugin_path, cmd, false); const char *err; if (!p) diff --git a/tests/test_misc.py b/tests/test_misc.py index d235837cb235..533c34b55a7d 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -821,7 +821,7 @@ def test_listconfigs(node_factory, bitcoind, chainparams): # Test one at a time. for c in configs.keys(): - if c.startswith('#') or c.startswith('plugins'): + if c.startswith('#') or c.startswith('plugins') or c == 'important-plugins': continue oneconfig = l1.rpc.listconfigs(config=c) assert(oneconfig[c] == configs[c]) From 0001750743d3ede91cb6f0029a32c2b1431f0485 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 30 Jul 2020 13:40:29 +0800 Subject: [PATCH 4/7] tests/test_plugin.py: Add test for --important-plugin. --- tests/plugins/suicidal_plugin.py | 14 ++++++++++++ tests/test_plugin.py | 38 ++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100755 tests/plugins/suicidal_plugin.py diff --git a/tests/plugins/suicidal_plugin.py b/tests/plugins/suicidal_plugin.py new file mode 100755 index 000000000000..75e58a53d443 --- /dev/null +++ b/tests/plugins/suicidal_plugin.py @@ -0,0 +1,14 @@ +#!/usr/bin/env python3 + +from pyln.client import Plugin +import os + +plugin = Plugin() + + +@plugin.method("die") +def die(): + os._exit(1) + + +plugin.run() diff --git a/tests/test_plugin.py b/tests/test_plugin.py index a8fc892c398f..c857bb0d759f 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1534,3 +1534,41 @@ def test_3847_repro(node_factory, bitcoind): # This call to paystatus would fail if the pay plugin crashed (it's # provided by the plugin) l1.rpc.paystatus(i1) + + +def test_important_plugin(node_factory): + # Cache it here. + pluginsdir = os.path.join(os.path.dirname(__file__), "plugins") + + # Check we fail if we cannot find the important plugin. + n = node_factory.get_node(options={"important-plugin": os.path.join(pluginsdir, "nonexistent")}, + may_fail=True, expect_fail=True, + allow_broken_log=True) + assert not n.daemon.running + assert n.daemon.is_in_stderr(r"error starting plugin '.*nonexistent'") + + # Check we exit if the important plugin dies. + n = node_factory.get_node(options={"important-plugin": os.path.join(pluginsdir, "fail_by_itself.py")}, + may_fail=True, expect_fail=True, + allow_broken_log=True) + + n.daemon.wait_for_log('fail_by_itself.py: Plugin marked as important, shutting down lightningd') + wait_for(lambda: not n.daemon.running) + + # Check if the important plugin is disabled, we run as normal. + n = node_factory.get_node(options=OrderedDict([("important-plugin", os.path.join(pluginsdir, "fail_by_itself.py")), + ("disable-plugin", "fail_by_itself.py")])) + # Make sure we can call into a plugin RPC (this is from `bcli`) even + # if fail_by_itself.py is disabled. + n.rpc.call("estimatefees", {}) + # Make sure we are still running. + assert n.daemon.running + n.stop() + + # Check if an important plugin dies later, we fail. + n = node_factory.get_node(options={"important-plugin": os.path.join(pluginsdir, "suicidal_plugin.py")}, + may_fail=True, allow_broken_log=True) + with pytest.raises(RpcError): + n.rpc.call("die", {}) + n.daemon.wait_for_log('suicidal_plugin.py: Plugin marked as important, shutting down lightningd') + wait_for(lambda: not n.daemon.running) From b79e37b127974e7e352c695db2719cb9a66258fe Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 30 Jul 2020 14:04:59 +0800 Subject: [PATCH 5/7] lightningd/plugin.c: Make builtin plugins important. Changelog-Changed: plugin: Builtin plugins are now marked as important, and if they crash, will cause C-lightning to stop as well. --- Makefile | 9 +++++++++ doc/lightningd-config.5 | 2 ++ doc/lightningd-config.5.md | 2 ++ lightningd/Makefile | 3 +++ lightningd/plugin.c | 15 ++++++++++----- tests/test_misc.py | 6 +++--- 6 files changed, 29 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index fed668a80cf2..a396fbf5fb51 100644 --- a/Makefile +++ b/Makefile @@ -268,6 +268,15 @@ include tools/Makefile include plugins/Makefile include tests/plugins/Makefile +# Generated from PLUGINS definition in plugins/Makefile +gen_list_of_builtin_plugins.h : plugins/Makefile Makefile + @echo GEN $@ + @rm -f $@ || true + @echo 'static const char *list_of_builtin_plugins[] = {' >> $@ + @echo '$(PLUGINS)' | sed 's@plugins/\([^ ]*\)@"\1",@g'>> $@ + @echo 'NULL' >> $@ + @echo '};' >> $@ + # Git doesn't maintain timestamps, so we only regen if git says we should. CHANGED_FROM_GIT = [ x"`git log $@ | head -n1`" != x"`git log $< | head -n1`" -o x"`git diff $<`" != x"" ] diff --git a/doc/lightningd-config.5 b/doc/lightningd-config.5 index 5f2a6d58b73b..6cc8ee1c907f 100644 --- a/doc/lightningd-config.5 +++ b/doc/lightningd-config.5 @@ -557,6 +557,8 @@ plugin stops for any reason (including via \fBlightning-plugin\fR(7) \fBstop\fR) C-lightning will also stop running\. This way, you can monitor crashes of important plugins by simply monitoring if C-lightning terminates\. +Built-in plugins, which are installed with \fBlightningd\fR(8), are automatically +considered important\. .SH BUGS diff --git a/doc/lightningd-config.5.md b/doc/lightningd-config.5.md index 316a7ac152a3..e71d9bb712b7 100644 --- a/doc/lightningd-config.5.md +++ b/doc/lightningd-config.5.md @@ -460,6 +460,8 @@ plugin stops for any reason (including via lightning-plugin(7) `stop`), C-lightning will also stop running. This way, you can monitor crashes of important plugins by simply monitoring if C-lightning terminates. +Built-in plugins, which are installed with lightningd(8), are automatically +considered important. BUGS ---- diff --git a/lightningd/Makefile b/lightningd/Makefile index 77aa6438ce7e..689c080537a3 100644 --- a/lightningd/Makefile +++ b/lightningd/Makefile @@ -137,6 +137,9 @@ LIGHTNINGD_HEADERS = $(LIGHTNINGD_HEADERS_NOGEN) $(LIGHTNINGD_HEADERS_GEN) $(WAL $(LIGHTNINGD_OBJS): $(LIGHTNINGD_HEADERS) +# Only the plugin component needs to depend on this header. +lightningd/plugin.o : gen_list_of_builtin_plugins.h + lightningd/gen_channel_state_names.h: lightningd/channel_state.h ccan/ccan/cdump/tools/cdump-enumstr ccan/ccan/cdump/tools/cdump-enumstr lightningd/channel_state.h > $@ diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 5fc4f6454fd2..028ce8eb6455 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -16,6 +16,9 @@ #include #include +/* Only this file can include this generated header! */ +# include + /* How many seconds may the plugin take to reply to the `getmanifest` * call? This is the maximum delay to `lightningd --help` and until * we can start the main `io_loop` to communicate with peers. If this @@ -1582,11 +1585,13 @@ struct log *plugin_get_log(struct plugin *plugin) void plugins_set_builtin_plugins_dir(struct plugins *plugins, const char *dir) { - /*~ The builtin-plugins dir does not need to exist, but - * we would error those anyway for our important built-in - * plugins. - */ - add_plugin_dir(plugins, dir, true); + /*~ Load the builtin plugins as important. */ + for (size_t i = 0; list_of_builtin_plugins[i]; ++i) + plugin_register(plugins, + take(path_join(NULL, dir, + list_of_builtin_plugins[i])), + NULL, + /* important = */ true); } struct plugin_destroyed { diff --git a/tests/test_misc.py b/tests/test_misc.py index 533c34b55a7d..5cc598204b11 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -832,9 +832,9 @@ def test_listconfigs_plugins(node_factory, bitcoind, chainparams): # assert that we have pay plugin and that plugins have a name and path configs = l1.rpc.listconfigs() - assert configs['plugins'] - assert len([p for p in configs['plugins'] if p['name'] == "pay"]) == 1 - for p in configs['plugins']: + assert configs['important-plugins'] + assert len([p for p in configs['important-plugins'] if p['name'] == "pay"]) == 1 + for p in configs['important-plugins']: assert p['name'] and len(p['name']) > 0 assert p['path'] and len(p['path']) > 0 assert os.path.isfile(p['path']) and os.access(p['path'], os.X_OK) From b9832f7234ebdcf3ceab4f866688fbf8373fa917 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Thu, 30 Jul 2020 15:07:32 +0800 Subject: [PATCH 6/7] tests/test_plugin.py: Test builtin plugins are important. --- tests/test_plugin.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/test_plugin.py b/tests/test_plugin.py index c857bb0d759f..49dcd202ffdc 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -10,11 +10,13 @@ expected_channel_features, account_balance, check_coin_moves, first_channel_id, check_coin_moves_idx ) +from pyln.testing.utils import TailableProc import json import os import pytest import re +import signal import sqlite3 import subprocess import time @@ -1572,3 +1574,25 @@ def test_important_plugin(node_factory): n.rpc.call("die", {}) n.daemon.wait_for_log('suicidal_plugin.py: Plugin marked as important, shutting down lightningd') wait_for(lambda: not n.daemon.running) + + # Check that if a builtin plugin dies, we fail. + n = node_factory.get_node(may_fail=True, allow_broken_log=True, + # The log message with the pay PID is printed + # very early in the logs. + start=False) + # Start the daemon directly, not via the node object n.start, + # because the normal n.daemon.start and n.start methods will + # wait for "Starting server with public key" and will execute + # getinfo, both of which are very much after plugins are + # started. + # And the PIDs of plugins are only seen at plugin startup. + TailableProc.start(n.daemon) + assert n.daemon.running + # Extract the pid of pay. + r = n.daemon.wait_for_log(r'started([0-9]*).*plugins/pay') + pidstr = re.search(r'.*started\(([0-9]*)\)', r).group(1) + # Kill pay. + os.kill(int(pidstr), signal.SIGKILL) + # node should die as well. + n.daemon.wait_for_log('pay: Plugin marked as important, shutting down lightningd') + wait_for(lambda: not n.daemon.running) From f69c694ed6433ac8468aee303776bec832bb0561 Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Tue, 4 Aug 2020 15:24:42 +0800 Subject: [PATCH 7/7] lightningd/plugin.c: Add a `--dev-builtin-plugins-unimportant` for developers who want to mess around with the builtin plugins. --- lightningd/options.c | 4 ++++ lightningd/plugin.c | 24 ++++++++++++++++++++++++ lightningd/plugin.h | 5 +++++ tests/test_plugin.py | 6 ++++++ 4 files changed, 39 insertions(+) diff --git a/lightningd/options.c b/lightningd/options.c index ba0099dd7b17..a1dad4f2248c 100644 --- a/lightningd/options.c +++ b/lightningd/options.c @@ -566,6 +566,10 @@ static void dev_register_opts(struct lightningd *ld) opt_register_noarg("--dev-no-version-checks", opt_set_bool, &ld->dev_no_version_checks, "Skip calling subdaemons with --version on startup"); + opt_register_early_noarg("--dev-builtin-plugins-unimportant", + opt_set_bool, + &ld->plugins->dev_builtin_plugins_unimportant, + "Make builtin plugins unimportant so you can plugin stop them."); } #endif /* DEVELOPER */ diff --git a/lightningd/plugin.c b/lightningd/plugin.c index 028ce8eb6455..d117b7cef405 100644 --- a/lightningd/plugin.c +++ b/lightningd/plugin.c @@ -57,6 +57,9 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book, p->json_cmds = tal_arr(p, struct command *, 0); p->blacklist = tal_arr(p, const char *, 0); p->shutdown = false; +#if DEVELOPER + p->dev_builtin_plugins_unimportant = false; +#endif /* DEVELOPER */ uintmap_init(&p->pending_requests); memleak_add_helper(p, memleak_help_pending_requests); @@ -1328,6 +1331,27 @@ void plugins_init(struct plugins *plugins) plugins->default_dir = path_join(plugins, plugins->ld->config_basedir, "plugins"); plugins_add_default_dir(plugins); +#if DEVELOPER + if (plugins->dev_builtin_plugins_unimportant) { + size_t i; + + log_debug(plugins->log, "Builtin plugins now unimportant"); + + /* For each builtin plugin, check for a matching plugin + * and make it unimportant. */ + for (i = 0; list_of_builtin_plugins[i]; ++i) { + const char *name = list_of_builtin_plugins[i]; + struct plugin *p; + list_for_each(&plugins->plugins, p, list) { + if (plugin_paths_match(p->cmd, name)) { + p->important = false; + break; + } + } + } + } +#endif /* DEVELOPER */ + setenv("LIGHTNINGD_PLUGIN", "1", 1); setenv("LIGHTNINGD_VERSION", version(), 1); diff --git a/lightningd/plugin.h b/lightningd/plugin.h index 5b494746055a..1e4338f841e1 100644 --- a/lightningd/plugin.h +++ b/lightningd/plugin.h @@ -110,6 +110,11 @@ struct plugins { /* Whether we are shutting down (`plugins_free` is called) */ bool shutdown; + +#if DEVELOPER + /* Whether builtin plugins should be overridden as unimportant. */ + bool dev_builtin_plugins_unimportant; +#endif /* DEVELOPER */ }; /* The value of a plugin option, which can have different types. diff --git a/tests/test_plugin.py b/tests/test_plugin.py index 49dcd202ffdc..a1830c9a773f 100644 --- a/tests/test_plugin.py +++ b/tests/test_plugin.py @@ -1596,3 +1596,9 @@ def test_important_plugin(node_factory): # node should die as well. n.daemon.wait_for_log('pay: Plugin marked as important, shutting down lightningd') wait_for(lambda: not n.daemon.running) + + +@unittest.skipIf(not DEVELOPER, "tests developer-only option.") +def test_dev_builtin_plugins_unimportant(node_factory): + n = node_factory.get_node(options={"dev-builtin-plugins-unimportant": None}) + n.rpc.plugin_stop(plugin="pay")