lib/nvmf: support ABI compatibility for spdk_nvmf_transport_opts

This patch is used to support ABI compatibility related with
spdk_nvmf_transport_opts structure. We add a field opts_size in
spdk_nvmf_transport_opts and change the related two functions.

Fixes issue: 1485

Change-Id: Ifed3dc482bbc8fb54eb7089f7a1931718682f214
Signed-off-by: Ziye Yang <ziye.yang@intel.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/5293
Community-CI: Broadcom CI
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Jacek Kalwas <jacek.kalwas@intel.com>

CHANGELOG.md

@@ -34,6 +34,10 @@ The SPDK nvmf target now supports async event notification for discovery log cha
 This allows the initiator to create persistent connection to discovery controller and
 be notified of any discovery log changes.
 
+An `opts_size`element was added in the `spdk_nvmf_transport_opts` structure
+to solve the ABI compatiblity issue between different SPDK version. And also add
+`opts_size` parameter in spdk_nvmf_transport_opts_init function.
+
 ### json
 
 A new API `spdk_jsonrpc_send_bool_response` was added to allow sending response for

include/spdk/nvmf.h

@@ -88,6 +88,14 @@ struct spdk_nvmf_transport_opts {
 	uint32_t	association_timeout;
 
 	const struct spdk_json_val *transport_specific;
+
+	/**
+	 * The size of spdk_nvmf_transport_opts according to the caller of this library is used for ABI
+	 * compatibility. The library uses this field to know how many fields in this
+	 * structure are valid. And the library will populate any remaining fields with default values.
+	 * After that, new added fields should be put after opts_size.
+	 */
+	size_t opts_size;
 };
 
 struct spdk_nvmf_poll_group_stat {
@@ -907,19 +915,21 @@ uint32_t spdk_nvmf_subsystem_get_max_nsid(struct spdk_nvmf_subsystem *subsystem)
  *
  * \param transport_name The transport type to create
  * \param opts The transport options (e.g. max_io_size)
+ * \param opts_size Must be set to sizeof(struct spdk_nvmf_transport_opts).
  *
  * \return bool. true if successful, false if transport type
  *	   not found.
  */
 bool
 spdk_nvmf_transport_opts_init(const char *transport_name,
-			      struct spdk_nvmf_transport_opts *opts);
+			      struct spdk_nvmf_transport_opts *opts, size_t opts_size);
 
 /**
  * Create a protocol transport
  *
  * \param transport_name The transport type to create
- * \param opts The transport options (e.g. max_io_size)
+ * \param opts The transport options (e.g. max_io_size). It should not be NULL, and opts_size
+ *        pointed in this structure should not be zero value.
  *
  * \return new transport or NULL if create fails
  */

lib/nvmf/nvmf_rpc.c

@@ -1961,7 +1961,7 @@ rpc_nvmf_create_transport(struct spdk_jsonrpc_request *request,
 	/* Initialize all the transport options (based on transport type) and decode the
 	 * parameters again to update any options passed in rpc create transport call.
 	 */
-	if (!spdk_nvmf_transport_opts_init(ctx->trtype, &ctx->opts)) {
+	if (!spdk_nvmf_transport_opts_init(ctx->trtype, &ctx->opts, sizeof(ctx->opts))) {
 		/* This can happen if user specifies PCIE transport type which isn't valid for
 		 * NVMe-oF.
 		 */

lib/nvmf/transport.c

@@ -107,6 +107,41 @@ spdk_nvmf_get_transport_name(struct spdk_nvmf_transport *transport)
 	return transport->ops->name;
 }
 
+static void nvmf_transport_opts_copy(struct spdk_nvmf_transport_opts *opts,
+				     struct spdk_nvmf_transport_opts *opts_src,
+				     size_t opts_size)
+{
+	assert(opts);
+	assert(opts_src);
+
+	opts->opts_size = opts_size;
+
+#define SET_FIELD(field) \
+	if (offsetof(struct spdk_nvmf_transport_opts, field) + sizeof(opts->field) <= opts_size) { \
+		opts->field = opts_src->field; \
+	} \
+
+	SET_FIELD(max_queue_depth);
+	SET_FIELD(max_qpairs_per_ctrlr);
+	SET_FIELD(in_capsule_data_size);
+	SET_FIELD(max_io_size);
+	SET_FIELD(io_unit_size);
+	SET_FIELD(max_aq_depth);
+	SET_FIELD(buf_cache_size);
+	SET_FIELD(num_shared_buffers);
+	SET_FIELD(dif_insert_or_strip);
+	SET_FIELD(abort_timeout_sec);
+	SET_FIELD(association_timeout);
+	SET_FIELD(transport_specific);
+
+	/* Do not remove this statement, you should always update this statement when you adding a new field,
+	 * and do not forget to add the SET_FIELD statement for your added field. */
+	SPDK_STATIC_ASSERT(sizeof(struct spdk_nvmf_transport_opts) == 56, "Incorrect size");
+
+#undef SET_FIELD
+#undef FILED_CHECK
+}
+
 struct spdk_nvmf_transport *
 spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transport_opts *opts)
 {
@@ -114,20 +149,32 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor
 	struct spdk_nvmf_transport *transport;
 	char spdk_mempool_name[MAX_MEMPOOL_NAME_LENGTH];
 	int chars_written;
+	struct spdk_nvmf_transport_opts opts_local = {};
+
+	if (!opts) {
+		SPDK_ERRLOG("opts should not be NULL\n");
+		return NULL;
+	}
+
+	if (!opts->opts_size) {
+		SPDK_ERRLOG("The opts_size in opts structure should not be zero\n");
+		return NULL;
+	}
 
 	ops = nvmf_get_transport_ops(transport_name);
 	if (!ops) {
 		SPDK_ERRLOG("Transport type '%s' unavailable.\n", transport_name);
 		return NULL;
 	}
+	nvmf_transport_opts_copy(&opts_local, opts, opts->opts_size);
 
-	if (opts->max_aq_depth < SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE) {
+	if (opts_local.max_aq_depth < SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE) {
 		SPDK_ERRLOG("max_aq_depth %u is less than minimum defined by NVMf spec, use min value\n",
-			    opts->max_aq_depth);
+			    opts_local.max_aq_depth);
-		opts->max_aq_depth = SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE;
+		opts_local.max_aq_depth = SPDK_NVMF_MIN_ADMIN_MAX_SQ_SIZE;
 	}
 
-	transport = ops->create(opts);
+	transport = ops->create(&opts_local);
 	if (!transport) {
 		SPDK_ERRLOG("Unable to create new transport of type %s\n", transport_name);
 		return NULL;
@@ -136,7 +183,8 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor
 	TAILQ_INIT(&transport->listeners);
 
 	transport->ops = ops;
-	transport->opts = *opts;
+	transport->opts = opts_local;
+
 	chars_written = snprintf(spdk_mempool_name, MAX_MEMPOOL_NAME_LENGTH, "%s_%s_%s", "spdk_nvmf",
 				 transport_name, "data");
 	if (chars_written < 0) {
@@ -146,8 +194,8 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor
 	}
 
 	transport->data_buf_pool = spdk_mempool_create(spdk_mempool_name,
-				   opts->num_shared_buffers,
+				   opts_local.num_shared_buffers,
-				   opts->io_unit_size + NVMF_DATA_BUFFER_ALIGNMENT,
+				   opts_local.io_unit_size + NVMF_DATA_BUFFER_ALIGNMENT,
 				   SPDK_MEMPOOL_DEFAULT_CACHE_SIZE,
 				   SPDK_ENV_SOCKET_ID_ANY);
 
@@ -511,9 +559,10 @@ nvmf_transport_qpair_abort_request(struct spdk_nvmf_qpair *qpair,
 
 bool
 spdk_nvmf_transport_opts_init(const char *transport_name,
-			      struct spdk_nvmf_transport_opts *opts)
+			      struct spdk_nvmf_transport_opts *opts, size_t opts_size)
 {
 	const struct spdk_nvmf_transport_ops *ops;
+	struct spdk_nvmf_transport_opts opts_local = {};
 
 	ops = nvmf_get_transport_ops(transport_name);
 	if (!ops) {
@@ -521,8 +570,21 @@ spdk_nvmf_transport_opts_init(const char *transport_name,
 		return false;
 	}
 
-	opts->association_timeout = NVMF_TRANSPORT_DEFAULT_ASSOCIATION_TIMEOUT_IN_MS;
+	if (!opts) {
-	ops->opts_init(opts);
+		SPDK_ERRLOG("opts should not be NULL\n");
+		return false;
+	}
+
+	if (!opts_size) {
+		SPDK_ERRLOG("opts_size inside opts should not be zero value\n");
+		return false;
+	}
+
+	opts_local.association_timeout = NVMF_TRANSPORT_DEFAULT_ASSOCIATION_TIMEOUT_IN_MS;
+	ops->opts_init(&opts_local);
+
+	nvmf_transport_opts_copy(opts, &opts_local, opts_size);
+
 	return true;
 }
 

test/unit/lib/nvmf/fc.c/fc_ut.c

@@ -293,6 +293,7 @@ create_transport_test(void)
 	ops->opts_init(&opts);
 
 	g_lld_init_called = false;
+	opts.opts_size = sizeof(opts);
 	g_nvmf_tprt = spdk_nvmf_transport_create("FC", &opts);
 	SPDK_CU_ASSERT_FATAL(g_nvmf_tprt != NULL);