lightningd: tighten interal json_stream API.

Move it closer to ccan/json_out, in preparation for using that as a
replacement.

In particular:

1. Add a 'quote' field in json_add_member.
2. json_add_member now always escapes if 'quote' is true.
3. json_member_direct is exposed to allow avoiding of escaping.
4. json_add_hex can use this, so no longer needs to be in json_stream.c.
5. We don't make JSON manually, but always use helpers.
6. We now flush the stream (wake reader) only when we close it, or mark
   command as pending.

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

lightningd/json.c

@@ -127,7 +127,7 @@ void json_add_short_channel_id(struct json_stream *response,
 			       const char *fieldname,
 			       const struct short_channel_id *scid)
 {
-	json_add_member(response, fieldname, "\"%dx%dx%d\"",
+	json_add_member(response, fieldname, true, "%dx%dx%d",
 			short_channel_id_blocknum(scid),
 			short_channel_id_txnum(scid),
 			short_channel_id_outnum(scid));
@@ -309,60 +309,78 @@ void json_add_address_internal(struct json_stream *response,
 
 void json_add_num(struct json_stream *result, const char *fieldname, unsigned int value)
 {
-	json_add_member(result, fieldname, "%u", value);
+	json_add_member(result, fieldname, false, "%u", value);
 }
 
 void json_add_double(struct json_stream *result, const char *fieldname, double value)
 {
-	json_add_member(result, fieldname, "%f", value);
+	json_add_member(result, fieldname, false, "%f", value);
 }
 
 void json_add_u64(struct json_stream *result, const char *fieldname,
 		  uint64_t value)
 {
-	json_add_member(result, fieldname, "%"PRIu64, value);
+	json_add_member(result, fieldname, false, "%"PRIu64, value);
 }
 
 void json_add_s64(struct json_stream *result, const char *fieldname,
 		  int64_t value)
 {
-	json_add_member(result, fieldname, "%"PRIi64, value);
+	json_add_member(result, fieldname, false, "%"PRIi64, value);
 }
 
 void json_add_u32(struct json_stream *result, const char *fieldname,
 		  uint32_t value)
 {
-	json_add_member(result, fieldname, "%d", value);
+	json_add_member(result, fieldname, false, "%u", value);
 }
 
 void json_add_s32(struct json_stream *result, const char *fieldname,
 		  int32_t value)
 {
-	json_add_member(result, fieldname, "%d", value);
+	json_add_member(result, fieldname, false, "%d", value);
 }
 
 void json_add_literal(struct json_stream *result, const char *fieldname,
 		      const char *literal, int len)
 {
-	json_add_member(result, fieldname, "%.*s", len, literal);
+	/* Literal may contain quotes, so bypass normal checks */
+	char *dest = json_member_direct(result, fieldname, strlen(literal));
+	if (dest)
+		memcpy(dest, literal, strlen(literal));
 }
 
 void json_add_string(struct json_stream *result, const char *fieldname, const char *value TAKES)
 {
-	struct json_escape *esc = json_partial_escape(NULL, value);
+	json_add_member(result, fieldname, true, "%s", value);
-
+	if (taken(value))
-	json_add_member(result, fieldname, "\"%s\"", esc->s);
+		tal_free(value);
-	tal_free(esc);
 }
 
 void json_add_bool(struct json_stream *result, const char *fieldname, bool value)
 {
-	json_add_member(result, fieldname, value ? "true" : "false");
+	json_add_member(result, fieldname, false, value ? "true" : "false");
 }
 
 void json_add_null(struct json_stream *stream, const char *fieldname)
 {
-	json_add_member(stream, fieldname, "null");
+	json_add_member(stream, fieldname, false, "null");
+}
+
+void json_add_hex(struct json_stream *js, const char *fieldname,
+		  const void *data, size_t len)
+{
+	/* Size without NUL term */
+	size_t hexlen = hex_str_size(len) - 1;
+	char *dest;
+
+	dest = json_member_direct(js, fieldname, 1 + hexlen + 1);
+	if (dest) {
+		dest[0] = '"';
+		if (!hex_encode(data, len, dest + 1, hexlen + 1))
+			abort();
+		dest[1+hexlen] = '"';
+	}
 }
 
 void json_add_hex_talarr(struct json_stream *result,
@@ -382,7 +400,15 @@ void json_add_tx(struct json_stream *result,
 void json_add_escaped_string(struct json_stream *result, const char *fieldname,
 			     const struct json_escape *esc TAKES)
 {
-	json_add_member(result, fieldname, "\"%s\"", esc->s);
+	/* Already escaped, don't re-escape! */
+	char *dest = json_member_direct(result,	fieldname,
+					1 + strlen(esc->s) + 1);
+
+	if (dest) {
+		dest[0] = '"';
+		memcpy(dest + 1, esc->s, strlen(esc->s));
+		dest[1+strlen(esc->s)] = '"';
+	}
 	if (taken(esc))
 		tal_free(esc);
 }
@@ -400,7 +426,7 @@ void json_add_amount_msat_only(struct json_stream *result,
 			  const char *msatfieldname,
 			  struct amount_msat msat)
 {
-	json_add_member(result, msatfieldname, "\"%s\"",
+	json_add_string(result, msatfieldname,
 			type_to_string(tmpctx, struct amount_msat, &msat));
 }
 
@@ -419,14 +445,14 @@ void json_add_amount_sat_only(struct json_stream *result,
 {
 	struct amount_msat msat;
 	if (amount_sat_to_msat(&msat, sat))
-		json_add_member(result, msatfieldname, "\"%s\"",
+		json_add_string(result, msatfieldname,
 				type_to_string(tmpctx, struct amount_msat, &msat));
 }
 
 void json_add_timeabs(struct json_stream *result, const char *fieldname,
 		      struct timeabs t)
 {
-	json_add_member(result, fieldname, "%" PRIu64 ".%03" PRIu64,
+	json_add_member(result, fieldname, false, "%" PRIu64 ".%03" PRIu64,
 			(u64)t.ts.tv_sec, (u64)t.ts.tv_nsec / 1000000);
 }
 

lightningd/json_stream.c

@@ -1,7 +1,9 @@
 #include <ccan/io/io.h>
   /* To reach into io_plan: not a public header! */
   #include <ccan/io/backend.h>
+#include <ccan/json_escape/json_escape.h>
 #include <ccan/str/hex/hex.h>
+#include <ccan/tal/str/str.h>
 #include <common/daemon.h>
 #include <common/utils.h>
 #include <lightningd/json.h>
@@ -94,15 +96,6 @@ bool json_stream_still_writing(const struct json_stream *js)
 	return js->writer != NULL;
 }
 
-void json_stream_close(struct json_stream *js, struct command *writer)
-{
-	/* FIXME: We use writer == NULL for malformed: make writer a void *?
-	 * I used to assert(writer); here. */
-	assert(js->writer == writer);
-
-	js->writer = NULL;
-}
-
 void json_stream_log_suppress(struct json_stream *js, const char *cmd_name)
 {
 	/* Really shouldn't be used for anything else */
@@ -146,67 +139,30 @@ static char *mkroom(struct json_stream *js, size_t len)
 	return membuf_space(&js->outbuf);
 }
 
-/* Also called when we're oom, so it will kill reader. */
+void json_stream_append(struct json_stream *js,
-static void js_written_some(struct json_stream *js)
+			const char *str, size_t len)
-{
-	/* Wake the stream reader. FIXME:  Could have a flag here to optimize */
-	io_wake(js);
-}
-
-void json_stream_append_part(struct json_stream *js, const char *str, size_t len)
 {
 	if (js->oom || !mkroom(js, len))
 		return;
 	memcpy(membuf_add(&js->outbuf, len), str, len);
-	js_written_some(js);
 }
 
-void json_stream_append(struct json_stream *js, const char *str)
+void json_stream_close(struct json_stream *js, struct command *writer)
 {
-	json_stream_append_part(js, str, strlen(str));
+	/* FIXME: We use writer == NULL for malformed: make writer a void *?
+	 * I used to assert(writer); here. */
+	assert(js->writer == writer);
+
+	json_stream_append(js, "\n\n", strlen("\n\n"));
+	json_stream_flush(js);
+	js->writer = NULL;
 }
 
-static void json_stream_append_vfmt(struct json_stream *js,
+/* Also called when we're oom, so it will kill reader. */
-				    const char *fmt, va_list ap)
+void json_stream_flush(struct json_stream *js)
 {
-	size_t fmtlen;
+	/* Wake the stream reader. FIXME:  Could have a flag here to optimize */
-	va_list ap2;
+	io_wake(js);
-
-	if (js->oom)
-		return;
-
-	/* Make a copy in case we need it below. */
-	va_copy(ap2, ap);
-
-	/* Try printing in place first. */
-	fmtlen = vsnprintf(membuf_space(&js->outbuf),
-			   membuf_num_space(&js->outbuf), fmt, ap);
-
-	/* Horrible subtlety: vsnprintf *will* NUL terminate, even if it means
-	 * chopping off the last character.  So if fmtlen ==
-	 * membuf_num_space(&jcon->outbuf), the result was truncated! */
-	if (fmtlen >= membuf_num_space(&js->outbuf)) {
-		/* Make room for NUL terminator, even though we don't want it */
-		char *p = mkroom(js, fmtlen + 1);
-		if (!p)
-			goto oom;
-		vsprintf(p, fmt, ap2);
-	}
-	membuf_added(&js->outbuf, fmtlen);
-
-oom:
-	js_written_some(js);
-	va_end(ap2);
-}
-
-void PRINTF_FMT(2,3)
-json_stream_append_fmt(struct json_stream *js, const char *fmt, ...)
-{
-	va_list ap;
-
-	va_start(ap, fmt);
-	json_stream_append_vfmt(js, fmt, ap);
-	va_end(ap);
 }
 
 static void check_fieldname(const struct json_stream *js,
@@ -226,10 +182,7 @@ static void check_fieldname(const struct json_stream *js,
 #endif
 }
 
-/* Caller must call js_written_some() if extra is non-zero returns non-NULL!
+char *json_member_direct(struct json_stream *js,
- * Can return NULL, beware:
- */
-static char *json_start_member(struct json_stream *js,
 			 const char *fieldname, size_t extra)
 {
 	char *dest;
@@ -291,61 +244,60 @@ static void js_unindent(struct json_stream *js, jsmntype_t type)
 
 void json_array_start(struct json_stream *js, const char *fieldname)
 {
-	char *dest = json_start_member(js, fieldname, 1);
+	char *dest = json_member_direct(js, fieldname, 1);
 	if (dest)
 		dest[0] = '[';
-	js_written_some(js);
 	js_indent(js, JSMN_ARRAY);
 }
 
 void json_array_end(struct json_stream *js)
 {
 	js_unindent(js, JSMN_ARRAY);
-	json_stream_append(js, "]");
+	json_stream_append(js, "]", 1);
 }
 
 void json_object_start(struct json_stream *js, const char *fieldname)
 {
-	char *dest = json_start_member(js, fieldname, 1);
+	char *dest = json_member_direct(js, fieldname, 1);
 	if (dest)
 		dest[0] = '{';
-	js_written_some(js);
 	js_indent(js, JSMN_OBJECT);
 }
 
 void json_object_end(struct json_stream *js)
 {
 	js_unindent(js, JSMN_OBJECT);
-	json_stream_append(js, "}");
+	json_stream_append(js, "}", 1);
 }
 
-void PRINTF_FMT(3,4)
+void json_add_member(struct json_stream *js,
-json_add_member(struct json_stream *js, const char *fieldname,
+		     const char *fieldname,
+		     bool quote,
 		     const char *fmt, ...)
 {
 	va_list ap;
+	char *str, *p;
 
-	json_start_member(js, fieldname, 0);
 	va_start(ap, fmt);
-	json_stream_append_vfmt(js, fmt, ap);
+	str = tal_vfmt(NULL, fmt, ap);
 	va_end(ap);
-}
 
-void json_add_hex(struct json_stream *js, const char *fieldname,
+	if (quote) {
-		  const void *data, size_t len)
+		struct json_escape *e = json_escape(NULL, take(str));
-{
-	/* Size without NUL term */
-	size_t hexlen = hex_str_size(len) - 1;
-	char *dest;
 
-	dest = json_start_member(js, fieldname, 1 + hexlen + 1);
+		p = json_member_direct(js, fieldname, strlen(e->s) + 2);
-	if (dest) {
+		if (!p)
-		dest[0] = '"';
+			return;
-		if (!hex_encode(data, len, dest + 1, hexlen + 1))
+		p[0] = p[1 + strlen(e->s)] = '"';
-			abort();
+		memcpy(p+1, e->s, strlen(e->s));
-		dest[1+hexlen] = '"';
+		tal_free(e);
+	} else {
+		p = json_member_direct(js, fieldname, strlen(str));
+		if (!p)
+			return;
+		memcpy(p, str, strlen(str));
+		tal_free(str);
 	}
-	js_written_some(js);
 }
 
 /* This is where we read the json_stream and write it to conn */

lightningd/json_stream.h

@@ -74,36 +74,37 @@ void json_object_end(struct json_stream *js);
  * json_stream_append - literally insert this string into the json_stream.
  * @js: the json_stream.
  * @str: the string.
- */
-void json_stream_append(struct json_stream *js, const char *str);
-
-/**
- * json_stream_append_part - literally insert part of string into json_stream.
- * @js: the json_stream.
- * @str: the string.
  * @len: the length to append (<= strlen(str)).
  */
-void json_stream_append_part(struct json_stream *js, const char *str,
+void json_stream_append(struct json_stream *js, const char *str, size_t len);
-			     size_t len);
-
-/**
- * json_stream_append_fmt - insert formatted string into the json_stream.
- * @js: the json_stream.
- * @fmt...: the printf-style format
- */
-PRINTF_FMT(2,3)
-void json_stream_append_fmt(struct json_stream *js, const char *fmt, ...);
 
 /**
  * json_add_member - add a generic member.
  * @js: the json_stream.
- * @fieldname: optional fieldname.
+ * @fieldname: fieldname (if in object), otherwise must be NULL.
+ * @quote: true if should be escaped and wrapped in "".
  * @fmt...: the printf-style format
+ *
+ * The resulting string from @fmt is escaped if quote is true:
+ * see json_member_direct to avoid quoting.
  */
-PRINTF_FMT(3,4)
+PRINTF_FMT(4,5)
-void json_add_member(struct json_stream *js, const char *fieldname,
+void json_add_member(struct json_stream *js,
+		     const char *fieldname,
+		     bool quote,
 		     const char *fmt, ...);
 
+/**
+ * json_member_direct - start a generic member.
+ * @js: the json_stream.
+ * @fieldname: fieldname (if in object), otherwise must be NULL.
+ * @extra: the space to reserve.
+ *
+ * Returns NULL if oom, otherwise returns a ptr to @extra bytes.
+ */
+char *json_member_direct(struct json_stream *js,
+			 const char *fieldname, size_t extra);
+
 /**
  * json_stream_output - start writing out a json_stream to this conn.
  * @js: the json_stream
@@ -127,4 +128,6 @@ struct io_plan *json_stream_output_(struct json_stream *js,
 							  void *arg),
 				    void *arg);
 
+void json_stream_flush(struct json_stream *js);
+
 #endif /* LIGHTNING_LIGHTNINGD_JSON_STREAM_H */

lightningd/jsonrpc.c

@@ -430,8 +430,9 @@ struct command_result *command_success(struct command *cmd,
 				       struct json_stream *result)
 {
 	assert(cmd);
-	assert(cmd->have_json_stream);
+	assert(cmd->json_stream == result);
-	json_stream_append(result, " } }\n\n");
+	json_object_end(result);
+	json_object_end(result);
 
 	return command_raw_complete(cmd, result);
 }
@@ -439,9 +440,10 @@ struct command_result *command_success(struct command *cmd,
 struct command_result *command_failed(struct command *cmd,
 				      struct json_stream *result)
 {
-	assert(cmd->have_json_stream);
+	assert(cmd->json_stream == result);
 	/* Have to close error */
-	json_stream_append(result, " } }\n\n");
+	json_object_end(result);
+	json_object_end(result);
 
 	return command_raw_complete(cmd, result);
 }
@@ -465,6 +467,11 @@ struct command_result *command_still_pending(struct command *cmd)
 {
 	notleak_with_children(cmd);
 	cmd->pending = true;
+
+	/* If we've started writing, wake reader. */
+	if (cmd->json_stream)
+		json_stream_flush(cmd->json_stream);
+
 	return &pending;
 }
 
@@ -475,12 +482,14 @@ static void json_command_malformed(struct json_connection *jcon,
 	/* NULL writer is OK here, since we close it immediately. */
 	struct json_stream *js = jcon_new_json_stream(jcon, jcon, NULL);
 
-	json_stream_append_fmt(js,
+	json_object_start(js, NULL);
-			       "{ \"jsonrpc\": \"2.0\", \"id\" : %s,"
+	json_add_string(js, "jsonrpc", "2.0");
-			       " \"error\" : "
+	json_add_literal(js, "id", id, strlen(id));
-			       "{ \"code\" : %d,"
+	json_object_start(js, "error");
-			       " \"message\" : \"%s\" } }\n\n",
+	json_add_member(js, "code", false, "%d", JSONRPC2_INVALID_REQUEST);
-			       id, JSONRPC2_INVALID_REQUEST, error);
+	json_add_string(js, "message", error);
+	json_object_end(js);
+	json_object_end(js);
 
 	json_stream_close(js, NULL);
 }
@@ -495,8 +504,8 @@ struct json_stream *json_stream_raw_for_cmd(struct command *cmd)
 	else
 		js = new_json_stream(cmd, cmd, NULL);
 
-	assert(!cmd->have_json_stream);
+	assert(!cmd->json_stream);
-	cmd->have_json_stream = true;
+	cmd->json_stream = js;
 	return js;
 }
 
@@ -514,16 +523,16 @@ static struct json_stream *json_start(struct command *cmd)
 {
 	struct json_stream *js = json_stream_raw_for_cmd(cmd);
 
-	json_stream_append_fmt(js, "{ \"jsonrpc\": \"2.0\", \"id\" : %s, ",
+	json_object_start(js, NULL);
-			       cmd->id);
+	json_add_string(js, "jsonrpc", "2.0");
+	json_add_literal(js, "id", cmd->id, strlen(cmd->id));
 	return js;
 }
 
 struct json_stream *json_stream_success(struct command *cmd)
 {
 	struct json_stream *r = json_start(cmd);
-	json_stream_append(r, "\"result\" : ");
+	json_object_start(r, "result");
-	json_object_start(r, NULL);
 	return r;
 }
 
@@ -531,15 +540,15 @@ struct json_stream *json_stream_fail_nodata(struct command *cmd,
 					    int code,
 					    const char *errmsg)
 {
-	struct json_stream *r = json_start(cmd);
+	struct json_stream *js = json_start(cmd);
-	struct json_escape *e = json_partial_escape(tmpctx, errmsg);
 
 	assert(code);
 
-	json_stream_append_fmt(r, " \"error\" : "
+	json_object_start(js, "error");
-			  "{ \"code\" : %d,"
+	json_add_member(js, "code", false, "%d", code);
-			  " \"message\" : \"%s\"", code, e->s);
+	json_add_string(js, "message", errmsg);
-	return r;
+
+	return js;
 }
 
 struct json_stream *json_stream_fail(struct command *cmd,
@@ -548,8 +557,7 @@ struct json_stream *json_stream_fail(struct command *cmd,
 {
 	struct json_stream *r = json_stream_fail_nodata(cmd, code, errmsg);
 
-	json_stream_append(r, ", \"data\" : ");
+	json_object_start(r, "data");
-	json_object_start(r, NULL);
 	return r;
 }
 
@@ -588,7 +596,7 @@ parse_request(struct json_connection *jcon, const jsmntok_t tok[])
 	c->jcon = jcon;
 	c->ld = jcon->ld;
 	c->pending = false;
-	c->have_json_stream = false;
+	c->json_stream = NULL;
 	c->id = tal_strndup(c,
 			    json_tok_full(jcon->buffer, id),
 			    json_tok_full_len(id));
@@ -1065,7 +1073,9 @@ void jsonrpc_notification_end(struct jsonrpc_notification *n)
 {
 	json_object_end(n->stream); /* closes '.params' */
 	json_object_end(n->stream); /* closes '.' */
-	json_stream_append(n->stream, "\n\n");
+
+	/* We guarantee to have \n\n at end of each response. */
+	json_stream_append(n->stream, "\n\n", strlen("\n\n"));
 }
 
 struct jsonrpc_request *jsonrpc_request_start_(
@@ -1101,7 +1111,9 @@ void jsonrpc_request_end(struct jsonrpc_request *r)
 {
 	json_object_end(r->stream); /* closes '.params' */
 	json_object_end(r->stream); /* closes '.' */
-	json_stream_append(r->stream, "\n\n");
+
+	/* We guarantee to have \n\n at end of each response. */
+	json_stream_append(r->stream, "\n\n", strlen("\n\n"));
 }
 
 /* We add this destructor as a canary to detect cmd failing. */

lightningd/jsonrpc.h

@@ -38,7 +38,7 @@ struct command {
 	/* Tell param() how to process the command */
 	enum command_mode mode;
 	/* Have we started a json stream already?  For debugging. */
-	bool have_json_stream;
+	struct json_stream *json_stream;
 };
 
 /**

lightningd/peer_control.c

@@ -493,7 +493,7 @@ static void json_add_sat_only(struct json_stream *result,
 	struct amount_msat msat;
 
 	if (amount_sat_to_msat(&msat, sat))
-		json_add_member(result, fieldname, "\"%s\"",
+		json_add_string(result, fieldname,
 				type_to_string(tmpctx, struct amount_msat, &msat));
 }
 

lightningd/plugin.c

@@ -591,20 +591,20 @@ static void json_stream_forward_change_id(struct json_stream *stream,
 	 * new_id into a string, or even worse, quote a string id
 	 * twice. */
 	size_t offset = idtok->type==JSMN_STRING?1:0;
-	json_stream_append_part(stream, buffer + toks->start,
+	json_stream_append(stream, buffer + toks->start,
 			   idtok->start - toks->start - offset);
 
-	json_stream_append(stream, new_id);
+	json_stream_append(stream, new_id, strlen(new_id));
-	json_stream_append_part(stream, buffer + idtok->end + offset,
+	json_stream_append(stream, buffer + idtok->end + offset,
 			   toks->end - idtok->end - offset);
 
 	/* We promise it will end in '\n\n' */
 	/* It's an object (with an id!): definitely can't be less that "{}" */
 	assert(toks->end - toks->start >= 2);
 	if (buffer[toks->end-1] != '\n')
-		json_stream_append(stream, "\n\n");
+		json_stream_append(stream, "\n\n", 2);
 	else if (buffer[toks->end-2] != '\n')
-		json_stream_append(stream, "\n");
+		json_stream_append(stream, "\n", 1);
 }
 
 static void plugin_rpcmethod_cb(const char *buffer,

lightningd/test/run-invoice-select-inchan.c

@@ -169,10 +169,6 @@ void json_add_hex_talarr(struct json_stream *result UNNEEDED,
 void json_add_log(struct json_stream *result UNNEEDED,
 		  const struct log_book *lr UNNEEDED, enum log_level minlevel UNNEEDED)
 { fprintf(stderr, "json_add_log called!\n"); abort(); }
-/* Generated stub for json_add_member */
-void json_add_member(struct json_stream *js UNNEEDED, const char *fieldname UNNEEDED,
-		     const char *fmt UNNEEDED, ...)
-{ fprintf(stderr, "json_add_member called!\n"); abort(); }
 /* Generated stub for json_add_node_id */
 void json_add_node_id(struct json_stream *response UNNEEDED,
 				const char *fieldname UNNEEDED,

wallet/test/run-wallet.c

@@ -244,10 +244,6 @@ void json_add_hex_talarr(struct json_stream *result UNNEEDED,
 void json_add_log(struct json_stream *result UNNEEDED,
 		  const struct log_book *lr UNNEEDED, enum log_level minlevel UNNEEDED)
 { fprintf(stderr, "json_add_log called!\n"); abort(); }
-/* Generated stub for json_add_member */
-void json_add_member(struct json_stream *js UNNEEDED, const char *fieldname UNNEEDED,
-		     const char *fmt UNNEEDED, ...)
-{ fprintf(stderr, "json_add_member called!\n"); abort(); }
 /* Generated stub for json_add_node_id */
 void json_add_node_id(struct json_stream *response UNNEEDED,
 				const char *fieldname UNNEEDED,