libplugin: don't spew datastore errors to LOG_DEBUG.

People get upset, especially as our "not found" error can be a bit hard to read! Signed-off-by: Rusty Russell <[email protected]> See-also: ElementsProject#5990
gudnuf · Feb 11, 2023 · 70aee52 · 70aee52
1 parent dd9400d
commit 70aee52
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 47 deletions.
diff --git a/plugins/autoclean.c b/plugins/autoclean.c
@@ -574,8 +574,11 @@ static const char *init(struct plugin *p,
 
 	cleantimer = plugin_timer(p, time_from_sec(cycle_seconds), do_clean_timer, NULL);
 
+	/* We don't care if this fails (it usually does, since entries
+	 * don't exist! */
 	for (enum subsystem i = 0; i < NUM_SUBSYSTEM; i++) {
-		rpc_scan_datastore_str(plugin, datastore_path(tmpctx, i, "num"),
+		rpc_scan_datastore_str(tmpctx, plugin,
+				       datastore_path(tmpctx, i, "num"),
 				       JSON_SCAN(json_to_u64, &total_cleaned[i]));
 	}
 

diff --git a/plugins/commando.c b/plugins/commando.c
@@ -989,6 +989,7 @@ static const char *init(struct plugin *p,
 			const char *buf UNUSED, const jsmntok_t *config UNUSED)
 {
 	struct secret rune_secret;
+	const char *err;
 
 	outgoing_commands = tal_arr(p, struct commando *, 0);
 	incoming_commands = tal_arr(p, struct commando *, 0);
@@ -1000,13 +1001,16 @@ static const char *init(struct plugin *p,
 #endif
 
 	rune_counter = tal(p, u64);
-	if (!rpc_scan_datastore_str(plugin, "commando/rune_counter",
-				    JSON_SCAN(json_to_u64, rune_counter)))
+	/* If this fails, it probably doesn't exist */
+	err = rpc_scan_datastore_str(tmpctx, plugin, "commando/rune_counter",
+				     JSON_SCAN(json_to_u64, rune_counter));
+	if (err)
 		rune_counter = tal_free(rune_counter);
 
 	/* Old python commando used to store secret */
-	if (!rpc_scan_datastore_hex(plugin, "commando/secret",
-				    JSON_SCAN(json_to_secret, &rune_secret))) {
+	err = rpc_scan_datastore_hex(tmpctx, plugin, "commando/secret",
+				     JSON_SCAN(json_to_secret, &rune_secret));
+	if (err) {
 		rpc_scan(plugin, "makesecret",
 			 /* $ i commando
 			  * 99 0x63 0143 0b1100011 'c'

diff --git a/plugins/libplugin.c b/plugins/libplugin.c
@@ -627,14 +627,14 @@ static void json_add_keypath(struct json_out *jout, const char *fieldname, const
 	json_out_end(jout, ']');
 }
 
-static bool rpc_scan_datastore(struct plugin *plugin,
-			       const char *path,
-			       const char *hex_or_string,
-			       va_list ap)
+static const char *rpc_scan_datastore(const tal_t *ctx,
+				      struct plugin *plugin,
+				      const char *path,
+				      const char *hex_or_string,
+				      va_list ap)
 {
 	const char *guide;
 	struct json_out *params;
-	const char *err;
 
 	params = json_out_new(NULL);
 	json_out_start(params, NULL, '{');
@@ -643,37 +643,35 @@ static bool rpc_scan_datastore(struct plugin *plugin,
 	json_out_finished(params);
 
 	guide = tal_fmt(tmpctx, "{datastore:[0:{%s:%%}]}", hex_or_string);
-	/* FIXME: Could be some other error, but that's probably a caller bug! */
-	err = rpc_scan_core(tmpctx, plugin, "listdatastore", take(params), guide, ap);
-	if (!err)
-		return true;
-	plugin_log(plugin, LOG_DBG, "listdatastore error %s: %s", path, err);
-	return false;
+	return rpc_scan_core(ctx, plugin, "listdatastore", take(params),
+			     guide, ap);
 }
 
-bool rpc_scan_datastore_str(struct plugin *plugin,
-			    const char *path,
-			    ...)
+const char *rpc_scan_datastore_str(const tal_t *ctx,
+				   struct plugin *plugin,
+				   const char *path,
+				   ...)
 {
-	bool ret;
+	const char *ret;
 	va_list ap;
 
 	va_start(ap, path);
-	ret = rpc_scan_datastore(plugin, path, "string", ap);
+	ret = rpc_scan_datastore(ctx, plugin, path, "string", ap);
 	va_end(ap);
 	return ret;
 }
 
 /* This variant scans the hex encoding, not the string */
-bool rpc_scan_datastore_hex(struct plugin *plugin,
-			    const char *path,
-			    ...)
+const char *rpc_scan_datastore_hex(const tal_t *ctx,
+				   struct plugin *plugin,
+				   const char *path,
+				   ...)
 {
-	bool ret;
+	const char *ret;
 	va_list ap;
 
 	va_start(ap, path);
-	ret = rpc_scan_datastore(plugin, path, "hex", ap);
+	ret = rpc_scan_datastore(ctx, plugin, path, "hex", ap);
 	va_end(ap);
 	return ret;
 }

diff --git a/plugins/libplugin.h b/plugins/libplugin.h
@@ -310,17 +310,19 @@ void rpc_scan(struct plugin *plugin,
 	      const char *guide,
 	      ...);
 
-/* Helper to scan datastore: can only be used in init callback.  *
- Returns false if field does not exist.  * path is /-separated.  Final
- arg is JSON_SCAN or JSON_SCAN_TAL.
+/* Helper to scan datastore: can only be used in init callback.  Returns error
+ * msg (usually meaning field does not exist), or NULL on success. path is
+ * /-separated.  Final arg is JSON_SCAN or JSON_SCAN_TAL.
  */
-bool rpc_scan_datastore_str(struct plugin *plugin,
-			    const char *path,
-			    ...);
+const char *rpc_scan_datastore_str(const tal_t *ctx,
+				   struct plugin *plugin,
+				   const char *path,
+				   ...);
 /* This variant scans the hex encoding, not the string */
-bool rpc_scan_datastore_hex(struct plugin *plugin,
-			    const char *path,
-			    ...);
+const char *rpc_scan_datastore_hex(const tal_t *ctx,
+				   struct plugin *plugin,
+				   const char *path,
+				   ...);
 
 /* This sets batching of database commitments */
 void rpc_enable_batching(struct plugin *plugin);

diff --git a/tests/plugins/test_libplugin.c b/tests/plugins/test_libplugin.c
@@ -131,7 +131,7 @@ static const char *init(struct plugin *p,
 			const char *buf UNUSED,
 			const jsmntok_t *config UNUSED)
 {
-	const char *name;
+	const char *name, *err_str, *err_hex;
 	const u8 *binname;
 
 	plugin_log(p, LOG_DBG, "test_libplugin initialised!");
@@ -143,19 +143,21 @@ static const char *init(struct plugin *p,
 		return "Disabled via selfdisable option";
 
 	/* Test rpc_scan_datastore funcs */
-	if (!rpc_scan_datastore_str(p, "test_libplugin/name",
-				    JSON_SCAN_TAL(tmpctx, json_strdup,
-						  &name)))
+	err_str = rpc_scan_datastore_str(tmpctx, p, "test_libplugin/name",
+					 JSON_SCAN_TAL(tmpctx, json_strdup,
+						       &name));
+	if (err_str)
 		name = NULL;
-	if (!rpc_scan_datastore_hex(p, "test_libplugin/name",
-				    JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex,
-						  &binname)))
+	err_hex = rpc_scan_datastore_hex(tmpctx, p, "test_libplugin/name",
+					 JSON_SCAN_TAL(tmpctx, json_tok_bin_from_hex,
+						       &binname));
+	if (err_hex)
 		binname = NULL;
 
 	plugin_log(p, LOG_INFORM, "String name from datastore: %s",
-		   name ? name : "NOT FOUND");
+		   name ? name : err_str);
 	plugin_log(p, LOG_INFORM, "Hex name from datastore: %s",
-		   binname ? tal_hex(tmpctx, binname) : "NOT FOUND");
+		   binname ? tal_hex(tmpctx, binname) : err_hex);
 
  	return NULL;
 }

diff --git a/tests/test_plugin.py b/tests/test_plugin.py
@@ -1499,8 +1499,8 @@ def test_libplugin(node_factory):
 
     # Test startup
     assert l1.daemon.is_in_log("test_libplugin initialised!")
-    assert l1.daemon.is_in_log("String name from datastore: NOT FOUND")
-    assert l1.daemon.is_in_log("Hex name from datastore: NOT FOUND")
+    assert l1.daemon.is_in_log("String name from datastore:.*token has no index 0")
+    assert l1.daemon.is_in_log("Hex name from datastore:.*token has no index 0")
 
     # This will look on datastore for default, won't find it.
     assert l1.rpc.call("helloworld") == {"hello": "NOT FOUND"}
@@ -1519,7 +1519,7 @@ def test_libplugin(node_factory):
     # yet whether strings are allowed:
     l1.daemon.wait_for_log(r"test_libplugin: [0-9]*\[OUT\]")
 
-    l1.daemon.wait_for_log("String name from datastore: NOT FOUND")
+    l1.daemon.wait_for_log("String name from datastore:.*object does not have member string")
     l1.daemon.wait_for_log("Hex name from datastore: 00010203")
 
     # Test commands