jsonrpc: dynamically allocate request object

This will be necessary when the lifetime of a request can exceed the
lifetime of the values stored in spdk_jsonrpc_server_conn.

Change-Id: Icd9772eb142e3f6ae69303aff1e12bc213f435a4
Signed-off-by: Daniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/368455
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>

lib/jsonrpc/jsonrpc_internal.h

@@ -42,12 +42,16 @@
 
 #define SPDK_JSONRPC_RECV_BUF_SIZE	(32 * 1024)
 #define SPDK_JSONRPC_SEND_BUF_SIZE	(32 * 1024)
+#define SPDK_JSONRPC_ID_MAX_LEN		128
 #define SPDK_JSONRPC_MAX_CONNS		64
 #define SPDK_JSONRPC_MAX_VALUES		1024
 
 struct spdk_jsonrpc_request {
 	struct spdk_jsonrpc_server_conn *conn;
-	const struct spdk_json_val *id;
+
+	/* Copy of request id value */
+	struct spdk_json_val id;
+	uint8_t id_data[SPDK_JSONRPC_ID_MAX_LEN];
 };
 
 struct spdk_jsonrpc_server_conn {

lib/jsonrpc/jsonrpc_server.c

@@ -59,14 +59,10 @@ static const struct spdk_json_object_decoder jsonrpc_request_decoders[] = {
 };
 
 static void
-parse_single_request(struct spdk_jsonrpc_server_conn *conn, struct spdk_json_val *values)
+parse_single_request(struct spdk_jsonrpc_request *request, struct spdk_json_val *values)
 {
 	bool invalid = false;
 	struct jsonrpc_request req = {};
-	struct spdk_jsonrpc_request request;
-
-	request.conn = conn;
-	request.id = NULL;
 
 	if (spdk_json_decode_object(values, jsonrpc_request_decoders,
 				    SPDK_COUNTOF(jsonrpc_request_decoders),
@@ -86,13 +82,22 @@ parse_single_request(struct spdk_jsonrpc_server_conn *conn, struct spdk_json_val
 	}
 
 	if (req.id) {
-		if (req.id->type != SPDK_JSON_VAL_STRING &&
+		if (req.id->type == SPDK_JSON_VAL_STRING ||
-		    req.id->type != SPDK_JSON_VAL_NUMBER &&
+		    req.id->type == SPDK_JSON_VAL_NUMBER) {
-		    req.id->type != SPDK_JSON_VAL_NULL) {
+			/* Copy value into request */
-			req.id = NULL;
+			if (req.id->len <= SPDK_JSONRPC_ID_MAX_LEN) {
+				request->id.type = req.id->type;
+				request->id.len = req.id->len;
+				memcpy(request->id.start, req.id->start, req.id->len);
+			} else {
+				SPDK_TRACELOG(SPDK_TRACE_RPC, "JSON-RPC request id too long (%u)\n",
+					      req.id->len);
+				invalid = true;
+			}
+		} else if (req.id->type == SPDK_JSON_VAL_NULL) {
+			request->id.type = SPDK_JSON_VAL_NULL;
+		} else  {
 			invalid = true;
-		} else {
-			request.id = req.id;
 		}
 	}
 
@@ -106,30 +111,39 @@ parse_single_request(struct spdk_jsonrpc_server_conn *conn, struct spdk_json_val
 
 done:
 	if (invalid) {
-		spdk_jsonrpc_server_handle_error(&request, SPDK_JSONRPC_ERROR_INVALID_REQUEST);
+		spdk_jsonrpc_server_handle_error(request, SPDK_JSONRPC_ERROR_INVALID_REQUEST);
 	} else {
-		spdk_jsonrpc_server_handle_request(&request, req.method, req.params);
+		spdk_jsonrpc_server_handle_request(request, req.method, req.params);
 	}
 }
 
 int
 spdk_jsonrpc_parse_request(struct spdk_jsonrpc_server_conn *conn, void *json, size_t size)
 {
-	struct spdk_jsonrpc_request error_request;
+	struct spdk_jsonrpc_request *request;
 	ssize_t rc;
 	void *end = NULL;
 
-	/* Build a request with id = NULL since we don't have a valid request ID */
-	error_request.conn = conn;
-	error_request.id = NULL;
-
 	/* Check to see if we have received a full JSON value. */
 	rc = spdk_json_parse(json, size, NULL, 0, &end, 0);
 	if (rc == SPDK_JSON_PARSE_INCOMPLETE) {
 		return 0;
-	} else if (rc < 0 || rc > SPDK_JSONRPC_MAX_VALUES) {
+	}
+
+	request = calloc(1, sizeof(*request));
+	if (request == NULL) {
+		SPDK_TRACELOG(SPDK_TRACE_RPC, "Out of memory allocating request\n");
+		return -1;
+	}
+
+	request->conn = conn;
+	request->id.start = request->id_data;
+	request->id.len = 0;
+	request->id.type = SPDK_JSON_VAL_INVALID;
+
+	if (rc < 0 || rc > SPDK_JSONRPC_MAX_VALUES) {
 		SPDK_TRACELOG(SPDK_TRACE_RPC, "JSON parse error\n");
-		spdk_jsonrpc_server_handle_error(&error_request, SPDK_JSONRPC_ERROR_PARSE_ERROR);
+		spdk_jsonrpc_server_handle_error(request, SPDK_JSONRPC_ERROR_PARSE_ERROR);
 
 		/*
 		 * Can't recover from parse error (no guaranteed resync point in streaming JSON).
@@ -143,20 +157,20 @@ spdk_jsonrpc_parse_request(struct spdk_jsonrpc_server_conn *conn, void *json, si
 			     SPDK_JSON_PARSE_FLAG_DECODE_IN_PLACE);
 	if (rc < 0 || rc > SPDK_JSONRPC_MAX_VALUES) {
 		SPDK_TRACELOG(SPDK_TRACE_RPC, "JSON parse error on second pass\n");
-		spdk_jsonrpc_server_handle_error(&error_request, SPDK_JSONRPC_ERROR_PARSE_ERROR);
+		spdk_jsonrpc_server_handle_error(request, SPDK_JSONRPC_ERROR_PARSE_ERROR);
 		return -1;
 	}
 
 	assert(end != NULL);
 
 	if (conn->values[0].type == SPDK_JSON_VAL_OBJECT_BEGIN) {
-		parse_single_request(conn, conn->values);
+		parse_single_request(request, conn->values);
 	} else if (conn->values[0].type == SPDK_JSON_VAL_ARRAY_BEGIN) {
 		SPDK_TRACELOG(SPDK_TRACE_RPC, "Got batch array (not currently supported)\n");
-		spdk_jsonrpc_server_handle_error(&error_request, SPDK_JSONRPC_ERROR_INVALID_REQUEST);
+		spdk_jsonrpc_server_handle_error(request, SPDK_JSONRPC_ERROR_INVALID_REQUEST);
 	} else {
 		SPDK_TRACELOG(SPDK_TRACE_RPC, "top-level JSON value was not array or object\n");
-		spdk_jsonrpc_server_handle_error(&error_request, SPDK_JSONRPC_ERROR_INVALID_REQUEST);
+		spdk_jsonrpc_server_handle_error(request, SPDK_JSONRPC_ERROR_INVALID_REQUEST);
 	}
 
 	return end - json;
@@ -176,10 +190,8 @@ begin_response(struct spdk_jsonrpc_server_conn *conn, const struct spdk_json_val
 	spdk_json_write_name(w, "jsonrpc");
 	spdk_json_write_string(w, "2.0");
 
-	if (id) {
+	spdk_json_write_name(w, "id");
-		spdk_json_write_name(w, "id");
+	spdk_json_write_val(w, id);
-		spdk_json_write_val(w, id);
-	}
 
 	return w;
 }
@@ -197,8 +209,15 @@ spdk_jsonrpc_begin_result(struct spdk_jsonrpc_request *request)
 {
 	struct spdk_json_write_ctx *w;
 
-	w = begin_response(request->conn, request->id);
+	if (request->id.type == SPDK_JSON_VAL_INVALID) {
+		/* Notification - no response required */
+		free(request);
+		return NULL;
+	}
+
+	w = begin_response(request->conn, &request->id);
 	if (w == NULL) {
+		free(request);
 		return NULL;
 	}
 
@@ -213,6 +232,7 @@ spdk_jsonrpc_end_result(struct spdk_jsonrpc_request *request, struct spdk_json_w
 	assert(w != NULL);
 
 	end_response(request->conn, w);
+	free(request);
 }
 
 void
@@ -220,18 +240,15 @@ spdk_jsonrpc_send_error_response(struct spdk_jsonrpc_request *request,
 				 int error_code, const char *msg)
 {
 	struct spdk_json_write_ctx *w;
-	struct spdk_json_val v_null;
-	const struct spdk_json_val *id;
 
-	id = request->id;
+	if (request->id.type == SPDK_JSON_VAL_INVALID) {
-	if (id == NULL) {
 		/* For error responses, if id is missing, explicitly respond with "id": null. */
-		v_null.type = SPDK_JSON_VAL_NULL;
+		request->id.type = SPDK_JSON_VAL_NULL;
-		id = &v_null;
 	}
 
-	w = begin_response(request->conn, id);
+	w = begin_response(request->conn, &request->id);
 	if (w == NULL) {
+		free(request);
 		return;
 	}
 
@@ -244,6 +261,7 @@ spdk_jsonrpc_send_error_response(struct spdk_jsonrpc_request *request,
 	spdk_json_write_object_end(w);
 
 	end_response(request->conn, w);
+	free(request);
 }
 
 SPDK_LOG_REGISTER_TRACE_FLAG("rpc", SPDK_TRACE_RPC)

test/unit/lib/jsonrpc/jsonrpc_server.c/jsonrpc_server_ut.c

@@ -45,6 +45,7 @@ struct req {
 	bool got_method;
 	bool got_id;
 	bool got_params;
+	struct spdk_jsonrpc_request *request;
 	struct spdk_json_val method;
 	struct spdk_json_val id;
 	struct spdk_json_val params[MAX_PARAMS];
@@ -98,6 +99,10 @@ static size_t g_num_reqs;
 	CU_ASSERT(g_cur_req->id.type == SPDK_JSON_VAL_STRING); \
 	CU_ASSERT(memcmp(g_cur_req->id.start, str, sizeof(str) - 1) == 0)
 
+#define REQ_ID_NULL() \
+	CU_ASSERT(g_cur_req->got_id); \
+	CU_ASSERT(g_cur_req->id.type == SPDK_JSON_VAL_NULL)
+
 #define REQ_ID_MISSING() \
 	CU_ASSERT(g_cur_req->got_id == false)
 
@@ -142,16 +147,21 @@ static size_t g_num_reqs;
 	CU_ASSERT(memcmp(g_params->start, str, g_params->len) == 0); \
 	g_params++
 
+#define FREE_REQUEST() \
+	free(g_reqs->request); \
+	g_reqs->request = NULL
+
 static void
 ut_handle(struct spdk_jsonrpc_request *request, int error, const struct spdk_json_val *method,
 	  const struct spdk_json_val *params)
 {
-	const struct spdk_json_val *id = request->id;
+	const struct spdk_json_val *id = &request->id;
 	struct req *r;
 
 	SPDK_CU_ASSERT_FATAL(g_num_reqs != MAX_REQS);
 	r = &g_reqs[g_num_reqs++];
 
+	r->request = request;
 	r->error = error;
 
 	if (method) {
@@ -169,7 +179,7 @@ ut_handle(struct spdk_jsonrpc_request *request, int error, const struct spdk_jso
 		r->got_params = false;
 	}
 
-	if (id) {
+	if (id && id->type != SPDK_JSON_VAL_INVALID) {
 		r->got_id = true;
 		r->id = *id;
 	} else {
@@ -180,6 +190,14 @@ ut_handle(struct spdk_jsonrpc_request *request, int error, const struct spdk_jso
 void
 spdk_jsonrpc_server_handle_error(struct spdk_jsonrpc_request *request, int error)
 {
+	/*
+	 * Map missing id to Null - this mirrors the behavior in the real
+	 * spdk_jsonrpc_server_handle_error() function.
+	 */
+	if (request->id.type == SPDK_JSON_VAL_INVALID) {
+		request->id.type = SPDK_JSON_VAL_NULL;
+	}
+
 	ut_handle(request, error, NULL, NULL);
 }
 
@@ -221,6 +239,7 @@ test_parse_request(void)
 	PARAM_NUM("42");
 	PARAM_NUM("23");
 	PARAM_ARRAY_END();
+	FREE_REQUEST();
 
 	/* rpc call with named parameters */
 	PARSE_PASS("{\"jsonrpc\": \"2.0\", \"method\": \"subtract\", \"params\": {\"subtrahend\": 23, \"minuend\": 42}, \"id\": 3}",
@@ -235,6 +254,7 @@ test_parse_request(void)
 	PARAM_NAME("minuend");
 	PARAM_NUM("42");
 	PARAM_OBJECT_END();
+	FREE_REQUEST();
 
 	/* notification */
 	PARSE_PASS("{\"jsonrpc\": \"2.0\", \"method\": \"update\", \"params\": [1,2,3,4,5]}", "");
@@ -249,20 +269,23 @@ test_parse_request(void)
 	PARAM_NUM("4");
 	PARAM_NUM("5");
 	PARAM_ARRAY_END();
+	FREE_REQUEST();
 
 	/* invalid JSON */
 	PARSE_FAIL("{\"jsonrpc\": \"2.0\", \"method\": \"foobar, \"params\": \"bar\", \"baz]");
 	REQ_BEGIN_INVALID(SPDK_JSONRPC_ERROR_PARSE_ERROR);
 	REQ_METHOD_MISSING();
-	REQ_ID_MISSING();
+	REQ_ID_NULL();
 	REQ_PARAMS_MISSING();
+	FREE_REQUEST();
 
 	/* invalid request (method must be a string; params must be array or object) */
 	PARSE_PASS("{\"jsonrpc\": \"2.0\", \"method\": 1, \"params\": \"bar\"}", "");
 	REQ_BEGIN_INVALID(SPDK_JSONRPC_ERROR_INVALID_REQUEST);
 	REQ_METHOD_MISSING();
-	REQ_ID_MISSING();
+	REQ_ID_NULL();
 	REQ_PARAMS_MISSING();
+	FREE_REQUEST();
 
 	/* batch, invalid JSON */
 	PARSE_FAIL(
@@ -272,15 +295,17 @@ test_parse_request(void)
 		"]");
 	REQ_BEGIN_INVALID(SPDK_JSONRPC_ERROR_PARSE_ERROR);
 	REQ_METHOD_MISSING();
-	REQ_ID_MISSING();
+	REQ_ID_NULL();
 	REQ_PARAMS_MISSING();
+	FREE_REQUEST();
 
 	/* empty array */
 	PARSE_PASS("[]", "");
 	REQ_BEGIN_INVALID(SPDK_JSONRPC_ERROR_INVALID_REQUEST);
 	REQ_METHOD_MISSING();
-	REQ_ID_MISSING();
+	REQ_ID_NULL();
 	REQ_PARAMS_MISSING();
+	FREE_REQUEST();
 
 	/* batch - not supported */
 	PARSE_PASS(
@@ -295,8 +320,9 @@ test_parse_request(void)
 
 	REQ_BEGIN_INVALID(SPDK_JSONRPC_ERROR_INVALID_REQUEST);
 	REQ_METHOD_MISSING();
-	REQ_ID_MISSING();
+	REQ_ID_NULL();
 	REQ_PARAMS_MISSING();
+	FREE_REQUEST();
 
 	free(conn);
 	free(server);
@@ -332,6 +358,7 @@ test_parse_request_streaming(void)
 	PARAM_ARRAY_BEGIN();
 	PARAM_NUM("1");
 	PARAM_ARRAY_END();
+	FREE_REQUEST();
 
 	/* Partial (but not invalid) requests - parse should not consume anything. */
 	strcpy(g_buf, "{\"jsonrpc\":\"2.0\",\"method\":\"b\",\"params\":[2],\"id\":2}");
@@ -342,10 +369,12 @@ test_parse_request_streaming(void)
 		int rc = spdk_jsonrpc_parse_request(conn, g_buf, i);
 		/* Partial request - no data consumed */
 		CU_ASSERT(rc == 0);
+		FREE_REQUEST();
 	}
 
 	/* Verify that full request can be parsed successfully */
 	CU_ASSERT(spdk_jsonrpc_parse_request(conn, g_buf, len) == (ssize_t)len);
+	FREE_REQUEST();
 
 	free(conn);
 	free(server);