Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The endless plugin saga (Part 1) #2075

Merged
merged 18 commits into from Nov 12, 2018
Merged

The endless plugin saga (Part 1) #2075

merged 18 commits into from Nov 12, 2018

Conversation

@cdecker
Copy link
Member

@cdecker cdecker commented Nov 5, 2018

This is the first part of the plugin subsystem. It includes the following steps:

  • Creates a sample plugin
  • Adds the --plugin cli option to register a plugin
  • Manages the plugin process
  • Sets up connectivity over stdin and stdout with the plugin and adds processing and multiplexing of JSON-RPC commands (lightningd as client to the plugin)
  • Splits the option parsing, asks plugins for any options they'd like to expose, and exposes them
  • Collects options for plugins and then configures the plugin.

Still to be done:

  • Documentation of the format and protocol that the plugins may talk
  • Event notifications from lightningd to the plugins (probably a separate PR)
  • Hooks that allow lightningd to call out to the plugin on automated actions to change the default behavior (hold HTLCs while waiting for delivery, ...) (definitely another PR).
@cdecker cdecker added this to the v0.6.3 milestone Nov 5, 2018
@cdecker cdecker self-assigned this Nov 5, 2018
@cdecker cdecker requested a review from rustyrussell Nov 5, 2018
Copy link
Contributor

@rustyrussell rustyrussell left a comment

This is really great! Some minor fixes; I think it should be committed pretty much as-is and we can work on further integration separately.

I'm wondering how to do built-in plugins we might ship: traverse a plugin dir by default? Allow --plugin=

or a new --plugin-dir?

char *cmd;
};

struct plugins {
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

I think this should simply be a struct list_head....

Copy link
Member Author

@cdecker cdecker Nov 7, 2018

We add some more state later on, which is why we have a struct in this case.

struct plugin *p;
size_t n = tal_count(plugins->plugins);
tal_resize(&plugins->plugins, n+1);
p = tal(plugins, struct plugin);
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

BTW, p = tal_arr_expand(&plugin->plugins); does this...

@@ -9,12 +11,27 @@ struct plugin {
int stdin, stdout;
pid_t pid;
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

Can we drop int stdin stdout now?

{
jsmntok_t *toks;
bool valid;
toks = json_parse_input(plugin->buffer, plugin->used, &valid);
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

This works, but lightning-cli does it more efficiently, by saving the jsmn_parser and simply continuing when new data comes in. Probably worth a note...


/* Stuff we read */
char *buffer;
size_t used, len_read;
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

This screams 'struct membuf'...

struct plugins {
struct plugin **plugins;
int pending_init;
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

size_t, since it's a count of things. intis used by old-school C programmers in place of bool, so I first wondered if this was a boolean...

struct plugins *p;
p = tal(ctx, struct plugins);
p->plugins = tal_arr(p, struct plugin *, 0);
p->log = log;
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

I think a plugin deserves its own log?

@@ -90,8 +106,8 @@ static bool plugin_read_json_one(struct plugin *plugin)
toks = json_parse_input(plugin->buffer, plugin->used, &valid);
if (!toks) {
if (!valid) {
/* FIXME (cdecker) Print error and kill the plugin */
return io_close(plugin->stdout_conn);
plugin_kill(plugin, "Failed to parse JSON response");
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

Log the actual output somewhere, for debugging?

Copy link
Member Author

@cdecker cdecker Nov 7, 2018

Should we roll that into log_io?

@@ -724,7 +724,7 @@ void register_opts(struct lightningd *ld)
{
opt_set_alloc(opt_allocfn, tal_reallocfn, tal_freefn);

opt_register_early_noarg("--help|-h", opt_lightningd_usage, ld,
opt_register_noarg("--help|-h", opt_lightningd_usage, ld,
"Print this message.");
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

This creates a problem; if you run lightningd --help it will create the lightning dir before printing anything. I think you need to leave it as an early arg, but have it set a flag. Then check if that flag is set and call opt_lightningd_usage after opt parsing is finshed?

/* FIXME(cdecker) Support numeric and boolean options as well */
if (!json_tok_streq(buffer, typetok, "string")) {
plugin_kill(plugin,
"Only \"string\" options currenctly supported");
Copy link
Contributor

@rustyrussell rustyrussell Nov 6, 2018

typo: currently...

I wonder if we really want to separate by type? After all, each option really is a 'string' in practice...

Copy link
Member Author

@cdecker cdecker Nov 6, 2018

Too much cryptocurrenctly in my brain :-)

@cdecker
Copy link
Member Author

@cdecker cdecker commented Nov 11, 2018

Ok, applied most of the changes that were pointed out by @rustyrussell, thanks for the thorough review 😃

Open questions that remain are:

  • membuf for JSON buffering: I'm working on a patch that'll do that, but currently I'm a bit stuck with the buffer size being wrong in the case a message was parsed, and trying to debug that.
  • Should we add the messages that flow between the plugin and the master to log_io? That'd address the debugging case for plugin devs.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Ack 98976f7

cdecker added 2 commits Nov 12, 2018
It does double duty to show how a simple plugin might work.
Copy link
Collaborator

@niftynei niftynei left a comment

lgtm, but you're probably going to want to redo a bunch of the comments etc to reflect the fact that you've changed init to getmanifest and configure to init.

weird aside, where does the value get set for an option? i'm assuming that's part of the opts code...

doc/plugins.md Outdated
@@ -0,0 +1,110 @@
# Plugins

Plugins are a simple, yet powerful, way to extend the functionality
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

nit: you don't need commas around yet powerful

doc/plugins.md Outdated
is implemented, the other features are under active development.*

The plugins may be written in any language, and communicate with
`lightningd` through their `stdin` and `stdout`. JSON-RPCv2 is used as
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

nit: what is the 'their' in terms of stdin / stdout? apologies if i'm being a bit dense here. maybe you can just say 'through stdin and stdout'.


/* Simple short args.*/
opt_register_noarg("-a", test_noarg, NULL, "All");
opt_register_early_noarg("-b|--blong", test_noarg, NULL, "All");
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

should also confirm that parse_early_args still works as intended (ie fails when unknown options are present)

*
* Initialization includes spinning up the plugins, reading their init messages,
* and registering the JSON-RPC passthrough and command line arguments. In order
* to read the init messages from the plugins we spin up our own io_loop that
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

todo: update this when you rename things

Copy link
Member Author

@cdecker cdecker Nov 12, 2018

Right, renamed the "init message" to "manifest" and "init messages" to "getmanifest reply"

const struct plugins *plugins)
{
struct plugin *p;
json_object_start(response, "plugin");
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

nit: should this be named plugins?


if (!typetok || !nametok || !desctok) {
plugin_kill(plugin,
"An option is missing either \"name\" or \"type\"");
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

\"description\" might be missing too.

popt->value = tal_strndup(plugin, buffer + defaulttok->start,
defaulttok->end - defaulttok->start);
popt->description = tal_fmt(
plugin, "%.*s (default: %s)", desctok->end - desctok->start,
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

really nice touch adding the default to the description 👍

const char *name, *sep;
char *conf = tal_fmt(tmpctx, "{\n \"options\": {");
list_for_each(&plugin->plugin_opts, opt, list) {

Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

nit: rm extra line?

p = tal(plugins, struct plugin);
list_add_tail(&plugins->plugins, &p->list);
p->plugins = plugins;
p->cmd = tal_strdup(p, path);

/* FIXME(cdecker): Referring to plugin by their registration
numner might not be that useful, come up with a naming scheme
Copy link
Collaborator

@niftynei niftynei Nov 12, 2018

nit: numner -> number

@cdecker
Copy link
Member Author

@cdecker cdecker commented Nov 12, 2018

Force pushed some of the changes, sorry. Here's the range-diff and the diff between the pre-force-push and the post-force-push:

 1:  d2f65b2b =  1:  d2f65b2b ccan: Update ccan modules to include incomplete option parsing
 2:  585dfd95 =  2:  585dfd95 plugins: Add simple helloworld plugin to test against
 3:  143d7c69 =  3:  143d7c69 plugin: Basic scaffolding for the plugin subsystem
 4:  6cc0de7f =  4:  6cc0de7f plugin: Add plugins to lightningd and register arguments
 5:  19140b5f =  5:  19140b5f plugin: Add listconfigs stub for the --plugin option
 6:  05b972f0 =  6:  05b972f0 plugin: Start each plugin and setup the connection to it
 7:  df26c82f !  7:  ac9fab76 plugin: Add request muxing to the plugin subsystem
    @@ -12,11 +12,6 @@
      #include <ccan/io/io.h>
      #include <ccan/list/list.h>
      #include <ccan/pipecmd/pipecmd.h>
    - #include <ccan/tal/str/str.h>
    -+#include <ccan/typesafe_cb/typesafe_cb.h>
    - #include <lightningd/json.h>
    - #include <unistd.h>
    - 
     @@
      	char *cmd;
      	struct io_conn *stdin_conn, *stdout_conn;
 -:  -------- >  8:  cbb8da1f fixup! plugin: Add request muxing to the plugin subsystem
 8:  2722c40c =  9:  bd51dff0 opts: Split early from non-early args so plugins can register theirs
 9:  54838b21 = 10:  d8335726 plugin: Add logs to plugin and add method to kill a plugin
10:  4c5c7ed3 ! 11:  a7100e00 plugin: Register plugin cli options
    @@ -49,7 +49,7 @@
     +#include <ccan/opt/opt.h>
      #include <ccan/pipecmd/pipecmd.h>
      #include <ccan/tal/str/str.h>
    - #include <ccan/typesafe_cb/typesafe_cb.h>
    + #include <lightningd/json.h>
     @@
      	const char *outbuf;
      
11:  4008fe4d = 12:  726978a4 plugin: Configure plugins once we've collected all cli options
12:  45e05f09 = 13:  5b71d33a plugin: Terminate objects with an empty line to signal end
13:  47f6a69a = 14:  c529fe93 plugin: Have the helloworld plugin accept a configure message
14:  78efd8c7 = 15:  e5495a19 plugin: Send the configure request once we collected all options
16:  776e000c = 16:  94bd05d1 plugin: Get rid of redundant stdin and stdout members
17:  440acad6 = 17:  83151bd1 plugin: Make the plugins a list
15:  9027c6c7 = 18:  2c7b413e docs: Add an initial draft of the plugin documentation
18:  1df9c286 = 19:  57777e72 plugin: Give each plugin their own log-prefix
 -:  -------- > 20:  f77f8456 fixup! plugin: Make the plugins a list
19:  c57a6a33 = 21:  c72e2984 fixup! docs: Add an initial draft of the plugin documentation
20:  8f5841ae = 22:  dba0dcb2 fixup! docs: Add an initial draft of the plugin documentation
 -:  -------- > 23:  dd75994c fixup! plugin: Basic scaffolding for the plugin subsystem
 -:  -------- > 24:  37f5b3c8 fixup! plugin: Make the plugins a list
 -:  -------- > 25:  094abe59 fixup! docs: Add an initial draft of the plugin documentation
 -:  -------- > 26:  36c01420 fixup! plugin: Register plugin cli options
 -:  -------- > 27:  121229fb fixup! plugin: Send the configure request once we collected all options
 -:  -------- > 28:  ca9c9e65 fixup! plugin: Give each plugin their own log-prefix

and here's the diff:

diff --git a/doc/plugins.md b/doc/plugins.md
index 9c1c6fc0..114c09f2 100644
--- a/doc/plugins.md
+++ b/doc/plugins.md
@@ -1,6 +1,6 @@
 # Plugins
 
-Plugins are a simple, yet powerful, way to extend the functionality
+Plugins are a simple yet powerful way to extend the functionality
 provided by c-lightning. They are subprocesses that are started by the
 main `lightningd` daemon and can interact with `lightningd` in a
 variety of ways:
@@ -19,10 +19,10 @@ variety of ways:
 *Notice: at the time of writing only command line option passthrough
 is implemented, the other features are under active development.*
 
-The plugins may be written in any language, and communicate with
-`lightningd` through their `stdin` and `stdout`. JSON-RPCv2 is used as
-protocol on top of the two streams, with the plugin acting as server
-and `lightningd` acting as client.
+A plugin may be written in any language, and communicates with
+`lightningd` through the plugin's `stdin` and `stdout`. JSON-RPCv2 is
+used as protocol on top of the two streams, with the plugin acting as
+server and `lightningd` acting as client.
 
 ## A day in the life of a plugin
 
diff --git a/lightningd/plugin.c b/lightningd/plugin.c
index fdad0726..ba76aeb9 100644
--- a/lightningd/plugin.c
+++ b/lightningd/plugin.c
@@ -6,7 +6,6 @@
 #include <ccan/opt/opt.h>
 #include <ccan/pipecmd/pipecmd.h>
 #include <ccan/tal/str/str.h>
-#include <ccan/typesafe_cb/typesafe_cb.h>
 #include <lightningd/json.h>
 #include <unistd.h>
 
@@ -93,7 +92,7 @@ void plugin_register(struct plugins *plugins, const char* path TAKES)
 	p->cmd = tal_strdup(p, path);
 
 	/* FIXME(cdecker): Referring to plugin by their registration
-	numner might not be that useful, come up with a naming scheme
+	number might not be that useful, come up with a naming scheme
 	that makes more sense. */
 	plugin_count++;
 	p->log = new_log(p, plugins->log_book, "plugin-%zu", plugin_count);
@@ -311,7 +310,7 @@ static bool plugin_opt_add(struct plugin *plugin, const char *buffer,
 
 	if (!typetok || !nametok || !desctok) {
 		plugin_kill(plugin,
-			    "An option is missing either \"name\" or \"type\"");
+			    "An option is missing either \"name\", \"description\" or \"type\"");
 		return false;
 	}
 
@@ -441,7 +440,6 @@ static void plugin_config(struct plugin *plugin)
 	const char *name, *sep;
 	char *conf = tal_fmt(tmpctx, "{\n  \"options\": {");
 	list_for_each(&plugin->plugin_opts, opt, list) {
-
 		/* Trim the `--` that we added before */
 		name = opt->name + 2;
 		/* Separator between elements in the same object */
@@ -465,7 +463,7 @@ void json_add_opt_plugins(struct json_stream *response,
 			  const struct plugins *plugins)
 {
 	struct plugin *p;
-	json_object_start(response, "plugin");
+	json_object_start(response, "plugins");
 	list_for_each(&plugins->plugins, p, list) {
 		json_object_start(response, p->cmd);
 		json_object_end(response);
diff --git a/lightningd/plugin.h b/lightningd/plugin.h
index 9da8636c..96ba978c 100644
--- a/lightningd/plugin.h
+++ b/lightningd/plugin.h
@@ -21,10 +21,11 @@ struct plugins *plugins_new(const tal_t *ctx, struct log_book *log_book);
 /**
  * Initialize the registered plugins.
  *
- * Initialization includes spinning up the plugins, reading their init messages,
- * and registering the JSON-RPC passthrough and command line arguments. In order
- * to read the init messages from the plugins we spin up our own io_loop that
- * exits once all plugins have responded.
+ * Initialization includes spinning up the plugins, reading their
+ * manifest, and registering the JSON-RPC passthrough and command line
+ * arguments. In order to read the getmanifest reply from the plugins
+ * we spin up our own io_loop that exits once all plugins have
+ * responded.
  */
 void plugins_init(struct plugins *plugins);

@niftynei
Copy link
Collaborator

@niftynei niftynei commented Nov 12, 2018

ACK ca9c9e6

cdecker added 15 commits Nov 12, 2018
The idea is that `plugin` is an early arg that is parsed (from command
line or the config file). We can then start the plugins and have them
tell us about the options they'd like to add to the mix, before we
actually parse them.

Signed-off-by: Christian Decker <@cdecker>
Also includes some sanity checks for the results returned by the
plugin, such as ensuring that the ID is as expected and that we have
either an error or a real result.
We also make `--help` a non-early arg so it allows for the plugins to
register their options before printing the help message. The options
themselves are stored in a separate struct inbetween them being
registered and them being forwarded to the plugin. Currently only
supports string options.

Signed-off-by: Christian Decker <@cdecker>
This is just meant as a hint to the plugin that the message is done,
and it can try to parse the buffer.

Signed-off-by: Christian Decker <@cdecker>
Just customizes the greeting, the actual sending is in the next commit.

Signed-off-by: Christian Decker <@cdecker>
This is the final step to get the plugins working. After parsing the
early options (including `--plugin`), then starting and asking the
plugins for options, and finally reading in the options we just
registered, we just need to assemble the options and send them over.

Signed-off-by: Christian Decker <@cdecker>
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
Suggested-by: Rusty Russell <@rustyrussell>
Signed-off-by: Christian Decker <@cdecker>
There is no good naming just yet, so we just number them 1 to n.
@cdecker
Copy link
Member Author

@cdecker cdecker commented Nov 12, 2018

Reapplying @niftynei's and @rustyrussell's ACK

ACK f379f31

@cdecker cdecker merged commit b02861b into ElementsProject:master Nov 12, 2018
2 checks passed
cdecker added a commit to cdecker/lightning that referenced this issue Nov 12, 2018
cdecker added a commit to cdecker/lightning that referenced this issue Nov 22, 2018
cdecker added a commit to cdecker/lightning that referenced this issue Nov 24, 2018
rustyrussell added a commit that referenced this issue Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants