plugin: Some cleanups that didn't make it into part 2 of the plugin saga

[cdecker]

• Changelog entry
• tal_arr_remove
• Timeout for getmanifest call
• Double free issue when killing a plugin.

[rustyrussell]

Minor cleanups only; looks v nice!

[rustyrussell]

See previous comment:

Also, I think we can now remove the if (cmd && and similar in find_cmd and jsonrpc_command_add?

If we don't clean this up now, we'll never do it, since everyone will wonder if there's some subtle reason they can be NULL. I'm assuming, of course, that there isn't!

[rustyrussell]

Needed this, so I fixed the other two and rebased...

[cdecker]

Fixups squashed, will merge as soon as CI passes.

[cdecker]

The rebase on top of master introduced a new error (failtimeout.py is supposed to fail, but if we use plugin-dir=contrib/plugins it'll get picked up automatically.

Hence I added commit 85c520c which ignores subdirectories and moved the failing plugins into a subdirectory.

range-diff pre- vs post-rebase:

-:  -------- > 1:  85c520c7 plugin: Ignore directories in the plugin-directory
1:  bc5e6d71 = 2:  bb69dba7 changelog: Add plugin JSON-RPC passthrough
2:  9fb4db7b = 3:  07662f22 common: Add tal_arr_remove helper
3:  f52f71bb = 4:  1f84a6d1 plugin: Add a timeout to the `getmanifest` call
4:  e50d8497 = 5:  62e9a115 plugin: Migrate request creation to json_stream
5:  6fca4da5 = 6:  8b353114 jsonrpc: Use tal_arr_remove instead of leaving NULL in the commands
6:  a9512f3b = 7:  bc929c4d plugin: Better cleanup when a plugin fails
7:  316c064c ! 8:  d6906eb0 plugin: Add a test for timeout and broken manifest
    @@ -7,10 +7,10 @@
     
         Signed-off-by: Christian Decker <decker.christian@gmail.com>
     
    -diff --git a/contrib/plugins/failtimeout.py b/contrib/plugins/failtimeout.py
    +diff --git a/contrib/plugins/fail/failtimeout.py b/contrib/plugins/fail/failtimeout.py
     new file mode 100755
     --- /dev/null
    -+++ b/contrib/plugins/failtimeout.py
    ++++ b/contrib/plugins/fail/failtimeout.py
     @@
     +#!/usr/bin/env python3
     +"""An example plugin that fails to answer to `getmanifest`
    @@ -82,8 +82,8 @@
     +
     +def test_failing_plugins():
     +    fail_plugins = [
    -+        'contrib/plugins/failtimeout.py',
    -+        'contrib/plugins/doesnotexist.py',
    ++        'contrib/plugins/fail/failtimeout.py',
    ++        'contrib/plugins/fail/doesnotexist.py',
     +    ]
     +
     +    for p in fail_plugins:

diff pre- vs. post-rebase:

diff --git a/contrib/plugins/failtimeout.py b/contrib/plugins/fail/failtimeout.py
similarity index 100%
rename from contrib/plugins/failtimeout.py
rename to contrib/plugins/fail/failtimeout.py
diff --git a/lightningd/plugin.c b/lightningd/plugin.c
index 5d003add..121ddf42 100644
--- a/lightningd/plugin.c
+++ b/lightningd/plugin.c
@@ -723,7 +723,11 @@ static const char *plugin_fullpath(const tal_t *ctx, const char *dir,
 	fullname = path_join(ctx, dir, basename);
 	if (stat(fullname, &st) != 0)
 		return tal_free(fullname);
-	if (!(st.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)))
+	if (!(st.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) || st.st_mode & S_IFDIR)
+		return tal_free(fullname);
+
+	/* Ignore directories, they have exec mode, but aren't executable. */
+	if (st.st_mode & S_IFDIR)
 		return tal_free(fullname);
 	return fullname;
 }
diff --git a/tests/test_plugin.py b/tests/test_plugin.py
index 68390ad9..8b85269a 100644
--- a/tests/test_plugin.py
+++ b/tests/test_plugin.py
@@ -81,8 +81,8 @@ def test_plugin_disable(node_factory):
 
 def test_failing_plugins():
     fail_plugins = [
-        'contrib/plugins/failtimeout.py',
-        'contrib/plugins/doesnotexist.py',
+        'contrib/plugins/fail/failtimeout.py',
+        'contrib/plugins/fail/doesnotexist.py',
     ]
 
     for p in fail_plugins:

[rustyrussell]

Ack d6906eb

[rustyrussell]

Reversing these tests makes more sense, but it's a trivial cleanup, so I'll apply this and do a separate PR.