commando: build ID of command based on the id they give us.

We change the libplugin API so commando can provide its own ID base.

This id chaining enables much nicer diagnostics!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

plugins/commando.c

@@ -365,6 +365,7 @@ static void try_command(struct node_id *peer,
 	const jsmntok_t *toks, *method, *params, *rune, *id;
 	const char *buf = (const char *)msg, *failmsg;
 	struct out_req *req;
+	const char *cmdid_prefix;
 
 	incoming->peer = *peer;
 	incoming->id = idnum;
@@ -395,10 +396,17 @@ static void try_command(struct node_id *peer,
 	}
 	rune = json_get_member(buf, toks, "rune");
 	id = json_get_member(buf, toks, "id");
-	if (!id && !deprecated_apis) {
-		commando_error(incoming, COMMANDO_ERROR_REMOTE,
-			       "missing id field");
-		return;
+	if (!id) {
+		if (!deprecated_apis) {
+			commando_error(incoming, COMMANDO_ERROR_REMOTE,
+				       "missing id field");
+			return;
+		}
+		cmdid_prefix = NULL;
+	} else {
+		cmdid_prefix = tal_fmt(tmpctx, "%.*s/",
+				       id->end - id->start,
+				       buf + id->start);
 	}
 
 	failmsg = check_rune(tmpctx, incoming, peer, buf, method, params, rune);
@@ -412,6 +420,7 @@ static void try_command(struct node_id *peer,
 	req = jsonrpc_request_whole_object_start(plugin, NULL,
 						 json_strdup(tmpctx, buf,
 							     method),
+						 cmdid_prefix,
 						 cmd_done, incoming);
 	if (params) {
 		size_t i;

plugins/libplugin.c

@@ -172,25 +172,30 @@ static void disable_request_cb(struct command *cmd, struct out_req *out)
 	out->cmd = NULL;
 }
 
-static const char *get_json_id(const tal_t *ctx,
-			       struct plugin *plugin,
-			       const char *cmd_id,
-			       const char *method)
+const char *json_id_prefix(const tal_t *ctx, const struct command *cmd)
 {
-	const char *prefix;
+	if (!cmd)
+		return "";
 
-	if (cmd_id) {
-		/* Strip quotes! */
-		if (strstarts(cmd_id, "\"")) {
-			assert(strlen(cmd_id) >= 2);
-			assert(strends(cmd_id, "\""));
-			prefix = tal_fmt(tmpctx, "%.*s/",
-					 (int)strlen(cmd_id) - 2, cmd_id + 1);
-		} else
-			prefix = tal_fmt(tmpctx, "%s/", cmd_id);
-	} else
-		prefix = "";
+	/* Notifications have no cmd->id, use methodname */
+	if (!cmd->id)
+		return tal_fmt(ctx, "%s/", cmd->methodname);
 
+	/* Strip quotes! */
+	if (strstarts(cmd->id, "\"")) {
+		assert(strlen(cmd->id) >= 2);
+		assert(strends(cmd->id, "\""));
+		return tal_fmt(ctx, "%.*s/",
+			       (int)strlen(cmd->id) - 2, cmd->id + 1);
+	}
+	return tal_fmt(ctx, "%s/", cmd->id);
+}
+
+static const char *append_json_id(const tal_t *ctx,
+				  struct plugin *plugin,
+				  const char *method,
+				  const char *prefix)
+{
 	return tal_fmt(ctx, "\"%s%s:%s#%"PRIu64"\"",
 		       prefix, plugin->id, method, plugin->next_outreq_id++);
 }
@@ -205,6 +210,7 @@ static void destroy_out_req(struct out_req *out_req, struct plugin *plugin)
 struct out_req *
 jsonrpc_request_start_(struct plugin *plugin, struct command *cmd,
 		       const char *method,
+		       const char *id_prefix,
 		       struct command_result *(*cb)(struct command *command,
 						    const char *buf,
 						    const jsmntok_t *result,
@@ -218,7 +224,7 @@ jsonrpc_request_start_(struct plugin *plugin, struct command *cmd,
 	struct out_req *out;
 
 	out = tal(cmd, struct out_req);
-	out->id = get_json_id(out, plugin, cmd ? cmd->id : NULL, method);
+	out->id = append_json_id(out, plugin, method, id_prefix);
 	out->cmd = cmd;
 	out->cb = cb;
 	out->errcb = errcb;
@@ -556,7 +562,7 @@ static const jsmntok_t *sync_req(const tal_t *ctx,
 	const jsmntok_t *contents;
 	int reqlen;
 	struct json_out *jout = json_out_new(tmpctx);
-	const char *id = get_json_id(tmpctx, plugin, "init", method);
+	const char *id = append_json_id(tmpctx, plugin, method, "init/");
 
 	json_out_start(jout, NULL, '{');
 	json_out_addstr(jout, "jsonrpc", "2.0");

plugins/libplugin.h

@@ -110,6 +110,7 @@ const struct feature_set *plugin_feature_set(const struct plugin *p);
 struct out_req *jsonrpc_request_start_(struct plugin *plugin,
 				       struct command *cmd,
 				       const char *method,
+				       const char *id_prefix,
 				       struct command_result *(*cb)(struct command *command,
 								    const char *buf,
 								    const jsmntok_t *result,
@@ -124,6 +125,7 @@ struct out_req *jsonrpc_request_start_(struct plugin *plugin,
  * "error" members. */
 #define jsonrpc_request_start(plugin, cmd, method, cb, errcb, arg)	\
 	jsonrpc_request_start_((plugin), (cmd), (method),		\
+		     json_id_prefix(tmpctx, (cmd)),			\
 		     typesafe_cb_preargs(struct command_result *, void *, \
 					 (cb), (arg),			\
 					 struct command *command,	\
@@ -139,8 +141,8 @@ struct out_req *jsonrpc_request_start_(struct plugin *plugin,
 
 /* This variant has callbacks received whole obj, not "result" or
  * "error" members.  It also doesn't start params{}. */
-#define jsonrpc_request_whole_object_start(plugin, cmd, method, cb, arg) \
-	jsonrpc_request_start_((plugin), (cmd), (method),		\
+#define jsonrpc_request_whole_object_start(plugin, cmd, method, id_prefix, cb, arg) \
+	jsonrpc_request_start_((plugin), (cmd), (method), (id_prefix),	\
 			       typesafe_cb_preargs(struct command_result *, void *, \
 						   (cb), (arg),		\
 						   struct command *command, \
@@ -470,6 +472,9 @@ struct createonion_response *json_to_createonion_response(const tal_t *ctx,
 struct route_hop *json_to_route(const tal_t *ctx, const char *buffer,
 				const jsmntok_t *toks);
 
+/* Create a prefix (ending in /) for this cmd_id, if any. */
+const char *json_id_prefix(const tal_t *ctx, const struct command *cmd);
+
 #if DEVELOPER
 struct htable;
 void plugin_set_memleak_handler(struct plugin *plugin,

plugins/test/run-route-overlong.c

@@ -107,6 +107,9 @@ void json_array_start(struct json_stream *js UNNEEDED, const char *fieldname UNN
 const jsmntok_t *json_get_member(const char *buffer UNNEEDED, const jsmntok_t tok[] UNNEEDED,
 				 const char *label UNNEEDED)
 { fprintf(stderr, "json_get_member called!\n"); abort(); }
+/* Generated stub for json_id_prefix */
+const char *json_id_prefix(const tal_t *ctx UNNEEDED, const struct command *cmd UNNEEDED)
+{ fprintf(stderr, "json_id_prefix called!\n"); abort(); }
 /* Generated stub for json_next */
 const jsmntok_t *json_next(const jsmntok_t *tok UNNEEDED)
 { fprintf(stderr, "json_next called!\n"); abort(); }
@@ -184,6 +187,7 @@ bool json_tok_streq(const char *buffer UNNEEDED, const jsmntok_t *tok UNNEEDED,
 struct out_req *jsonrpc_request_start_(struct plugin *plugin UNNEEDED,
 				       struct command *cmd UNNEEDED,
 				       const char *method UNNEEDED,
+				       const char *id_prefix UNNEEDED,
 				       struct command_result *(*cb)(struct command *command UNNEEDED,
 								    const char *buf UNNEEDED,
 								    const jsmntok_t *result UNNEEDED,

tests/test_plugin.py

@@ -2608,7 +2608,8 @@ def test_plugin_shutdown(node_factory):
 
 
 def test_commando(node_factory, executor):
-    l1, l2 = node_factory.line_graph(2, fundchannel=False)
+    l1, l2 = node_factory.line_graph(2, fundchannel=False,
+                                     opts={'log-level': 'io'})
 
     # Nothing works until we've issued a rune.
     fut = executor.submit(l2.rpc.call, method='commando',
@@ -2634,6 +2635,11 @@ def test_commando(node_factory, executor):
     assert len(res['peers']) == 1
     assert res['peers'][0]['id'] == l2.info['id']
 
+    # Check JSON id is as expected (unfortunately pytest does not use a reliable name
+    # for itself: with -k it calls itself `-c` here, instead of `pytest`).
+    l2.daemon.wait_for_log(r'plugin-commando: "[^:/]*:commando#[0-9]*/cln:commando#[0-9]*"\[OUT\]')
+    l1.daemon.wait_for_log(r'jsonrpc#[0-9]*: "[^:/]*:commando#[0-9]*/cln:commando#[0-9]*/commando:listpeers#[0-9]*"\[IN\]')
+
     res = l2.rpc.call(method='commando',
                       payload={'peer_id': l1.info['id'],
                                'rune': rune,