lightningd: have plugin-disable be more persistent.

The previous implementation was a bit lazy: in particular, since we didn't
remember the disabled plugins, we would load them on rescan.

Changelog-Changed: config: the `plugin-disable` option works even if specified before the plugin is found.

doc/lightningd-config.5

@@ -528,9 +528,11 @@ normal effect\.
 
 
  \fBdisable-plugin\fR=\fIPLUGIN\fR
-If \fIPLUGIN\fR contains a /, plugins with the same path as \fIPLUGIN\fR are
-disabled\. Otherwise, any plugin with that base name is disabled,
-whatever directory it is in\.
+If \fIPLUGIN\fR contains a /, plugins with the same path as \fIPLUGIN\fR will
+not be loaded at startup\. Otherwise, no plugin with that base name will
+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\.
 
 .SH BUGS
 

doc/lightningd-config.5.md

@@ -434,9 +434,11 @@ including the default built-in plugin directory. You can still add
 normal effect.
 
  **disable-plugin**=*PLUGIN*
-If *PLUGIN* contains a /, plugins with the same path as *PLUGIN* are
-disabled. Otherwise, any plugin with that base name is disabled,
-whatever directory it is in.
+If *PLUGIN* contains a /, plugins with the same path as *PLUGIN* will
+not be loaded at startup. Otherwise, no plugin with that base name will
+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`.
 
 BUGS
 ----

lightningd/options.c

@@ -343,15 +343,18 @@ static char *opt_add_proxy_addr(const char *arg, struct lightningd *ld)
 
 static char *opt_add_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);
 	return NULL;
 }
 
 static char *opt_disable_plugin(const char *arg, struct lightningd *ld)
 {
-	if (plugin_remove(ld->plugins, arg))
-		return NULL;
-	return tal_fmt(NULL, "Could not find plugin %s", arg);
+	plugin_blacklist(ld->plugins, arg);
+	return NULL;
 }
 
 static char *opt_add_plugin_dir(const char *arg, struct lightningd *ld)

lightningd/plugin.c

@@ -52,6 +52,7 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book,
 	p->ld = ld;
 	p->startup = true;
 	p->json_cmds = tal_arr(p, struct command *, 0);
+	p->blacklist = tal_arr(p, const char *, 0);
 	uintmap_init(&p->pending_requests);
 	memleak_add_helper(p, memleak_help_pending_requests);
 
@@ -175,19 +176,30 @@ bool plugin_paths_match(const char *cmd, const char *name)
 	}
 }
 
-bool plugin_remove(struct plugins *plugins, const char *name)
+void plugin_blacklist(struct plugins *plugins, const char *name)
 {
 	struct plugin *p, *next;
-	bool removed = false;
 
 	list_for_each_safe(&plugins->plugins, p, next, list) {
 		if (plugin_paths_match(p->cmd, name)) {
+			log_info(plugins->log, "%s: disabled via disable-plugin",
+				 p->cmd);
 			list_del_from(&plugins->plugins, &p->list);
 			tal_free(p);
-			removed = true;
 		}
 	}
-	return removed;
+
+	tal_arr_expand(&plugins->blacklist,
+		       tal_strdup(plugins->blacklist, name));
+}
+
+bool plugin_blacklisted(struct plugins *plugins, const char *name)
+{
+	for (size_t i = 0; i < tal_count(plugins->blacklist); i++)
+		if (plugin_paths_match(name, plugins->blacklist[i]))
+			return true;
+
+	return false;
 }
 
 void plugin_kill(struct plugin *plugin, const char *msg)
@@ -1179,7 +1191,12 @@ char *add_plugin_dir(struct plugins *plugins, const char *dir, bool error_ok)
 		if (streq(di->d_name, ".") || streq(di->d_name, ".."))
 			continue;
 		fullpath = plugin_fullpath(tmpctx, dir, di->d_name);
-		if (fullpath) {
+		if (!fullpath)
+			continue;
+		if (plugin_blacklisted(plugins, fullpath)) {
+			log_info(plugins->log, "%s: disabled via disable-plugin",
+				 fullpath);
+		} else {
 			p = plugin_register(plugins, fullpath, NULL);
 			if (!p && !error_ok)
 				return tal_fmt(NULL, "Failed to register %s: %s",

lightningd/plugin.h

@@ -100,6 +100,9 @@ struct plugins {
 
 	/* If there are json commands waiting for plugin resolutions. */
 	struct command **json_cmds;
+
+	/* Blacklist of plugins from --disable-plugin */
+	const char **blacklist;
 };
 
 /* The value of a plugin option, which can have different types.
@@ -191,10 +194,18 @@ bool plugin_paths_match(const char *cmd, const char *name);
  * @param plugins: Plugin context
  * @param arg: The basename or fullname of the executable for this plugin
  */
-bool plugin_remove(struct plugins *plugins, const char *name);
+void plugin_blacklist(struct plugins *plugins, const char *name);
 
 /**
- * Kick of initialization of a plugin.
+ * Is a plugin disabled?.
+ *
+ * @param plugins: Plugin context
+ * @param arg: The basename or fullname of the executable for this plugin
+ */
+bool plugin_blacklisted(struct plugins *plugins, const char *name);
+
+/**
+ * Kick off initialization of a plugin.
  *
  * Returns error string, or NULL.
  */

tests/test_plugin.py

@@ -273,13 +273,14 @@ def test_plugin_command(node_factory):
 def test_plugin_disable(node_factory):
     """--disable-plugin works"""
     plugin_dir = os.path.join(os.getcwd(), 'contrib/plugins')
-    # We need plugin-dir before disable-plugin!
+    # We used to need plugin-dir before disable-plugin!
     n = node_factory.get_node(options=OrderedDict([('plugin-dir', plugin_dir),
                                                    ('disable-plugin',
                                                     '{}/helloworld.py'
                                                     .format(plugin_dir))]))
     with pytest.raises(RpcError):
         n.rpc.hello(name='Sun')
+    assert n.daemon.is_in_log('helloworld.py: disabled via disable-plugin')
 
     # Also works by basename.
     n = node_factory.get_node(options=OrderedDict([('plugin-dir', plugin_dir),
@@ -287,6 +288,39 @@ def test_plugin_disable(node_factory):
                                                     'helloworld.py')]))
     with pytest.raises(RpcError):
         n.rpc.hello(name='Sun')
+    assert n.daemon.is_in_log('helloworld.py: disabled via disable-plugin')
+
+    # Other order also works!
+    n = node_factory.get_node(options=OrderedDict([('disable-plugin',
+                                                    'helloworld.py'),
+                                                   ('plugin-dir', plugin_dir)]))
+    with pytest.raises(RpcError):
+        n.rpc.hello(name='Sun')
+    assert n.daemon.is_in_log('helloworld.py: disabled via disable-plugin')
+
+    # Both orders of explicit specification work.
+    n = node_factory.get_node(options=OrderedDict([('disable-plugin',
+                                                    'helloworld.py'),
+                                                   ('plugin',
+                                                    '{}/helloworld.py'
+                                                    .format(plugin_dir))]))
+    with pytest.raises(RpcError):
+        n.rpc.hello(name='Sun')
+    assert n.daemon.is_in_log('helloworld.py: disabled via disable-plugin')
+
+    # Both orders of explicit specification work.
+    n = node_factory.get_node(options=OrderedDict([('plugin',
+                                                    '{}/helloworld.py'
+                                                    .format(plugin_dir)),
+                                                   ('disable-plugin',
+                                                    'helloworld.py')]))
+    with pytest.raises(RpcError):
+        n.rpc.hello(name='Sun')
+    assert n.daemon.is_in_log('helloworld.py: disabled via disable-plugin')
+
+    # Still disabled if we load directory.
+    n.rpc.plugin_startdir(directory=os.path.join(os.getcwd(), "contrib/plugins"))
+    n.daemon.wait_for_log('helloworld.py: disabled via disable-plugin')
 
 
 def test_plugin_hook(node_factory, executor):