lib: fix init. use of nb_context to be by value not by reference

Pass context argument by value on initialization to be clear that the
value is used/saved but not a pointer to the value. Previously the
northbound code was incorrectly holding a pointer to stack allocated
context structs.

However, the structure definition also had some musings (ifdef'd out
code) and a comment that might be taken to imply that user data could
follow the structure and thus be maintained by the code; it won't; so it
can't; so get rid of the disabled misleading code/text from the
structure definition.

The common use case worked b/c the transaction which cached the pointer
was created and freed inside a single function
call (`nb_condidate_commit`) that executed below the stack allocation.

All other use cases (grpc, confd, sysrepo, and -- coming soon -- mgmtd)
were bugs.

Signed-off-by: Christian Hopps <chopps@labn.net>

9 files changed, 24 insertions, 42 deletions

diff --git a/lib/libfrr.c b/lib/libfrr.c
index 0467dc1d7..d1b7dd133 100644
--- a/lib/libfrr.c
+++ b/lib/libfrr.c
@@ -992,7 +992,7 @@ static void frr_config_read_in(struct thread *t)
 		int ret;
 
 		context.client = NB_CLIENT_CLI;
-		ret = nb_candidate_commit(&context, vty_shared_candidate_config,
+		ret = nb_candidate_commit(context, vty_shared_candidate_config,
 					  true, "Read configuration file", NULL,
 					  errmsg, sizeof(errmsg));
 		if (ret != NB_OK && ret != NB_ERR_NO_CHANGES)
diff --git a/lib/northbound.c b/lib/northbound.c
index b755264be..6f2c522a2 100644
--- a/lib/northbound.c
+++ b/lib/northbound.c
@@ -61,7 +61,7 @@ static int nb_callback_configuration(struct nb_context *context,
 				     struct nb_config_change *change,
 				     char *errmsg, size_t errmsg_len);
 static struct nb_transaction *
-nb_transaction_new(struct nb_context *context, struct nb_config *config,
+nb_transaction_new(struct nb_context context, struct nb_config *config,
 		   struct nb_config_cbs *changes, const char *comment,
 		   char *errmsg, size_t errmsg_len);
 static void nb_transaction_free(struct nb_transaction *transaction);
@@ -835,7 +835,7 @@ int nb_candidate_validate(struct nb_context *context,
 	return ret;
 }
 
-int nb_candidate_commit_prepare(struct nb_context *context,
+int nb_candidate_commit_prepare(struct nb_context context,
 				struct nb_config *candidate,
 				const char *comment,
 				struct nb_transaction **transaction,
@@ -860,9 +860,8 @@ int nb_candidate_commit_prepare(struct nb_context *context,
 		return NB_ERR_NO_CHANGES;
 	}
 
-	if (nb_candidate_validate_code(context, candidate, &changes, errmsg,
-				       errmsg_len)
-	    != NB_OK) {
+	if (nb_candidate_validate_code(&context, candidate, &changes, errmsg,
+				       errmsg_len) != NB_OK) {
 		flog_warn(EC_LIB_NB_CANDIDATE_INVALID,
 			  "%s: failed to validate candidate configuration",
 			  __func__);
@@ -913,7 +912,7 @@ void nb_candidate_commit_apply(struct nb_transaction *transaction,
 	nb_transaction_free(transaction);
 }
 
-int nb_candidate_commit(struct nb_context *context, struct nb_config *candidate,
+int nb_candidate_commit(struct nb_context context, struct nb_config *candidate,
 			bool save_transaction, const char *comment,
 			uint32_t *transaction_id, char *errmsg,
 			size_t errmsg_len)
@@ -1411,13 +1410,13 @@ static int nb_callback_configuration(struct nb_context *context,
 }
 
 static struct nb_transaction *
-nb_transaction_new(struct nb_context *context, struct nb_config *config,
+nb_transaction_new(struct nb_context context, struct nb_config *config,
 		   struct nb_config_cbs *changes, const char *comment,
 		   char *errmsg, size_t errmsg_len)
 {
 	struct nb_transaction *transaction;
 
-	if (nb_running_lock_check(context->client, context->user)) {
+	if (nb_running_lock_check(context.client, context.user)) {
 		strlcpy(errmsg,
 			"running configuration is locked by another client",
 			errmsg_len);
@@ -1469,7 +1468,7 @@ static int nb_transaction_process(enum nb_event event,
 			break;
 
 		/* Call the appropriate callback. */
-		ret = nb_callback_configuration(transaction->context, event,
+		ret = nb_callback_configuration(&transaction->context, event,
 						change, errmsg, errmsg_len);
 		switch (event) {
 		case NB_EV_PREPARE:
@@ -1584,7 +1583,7 @@ static void nb_transaction_apply_finish(struct nb_transaction *transaction,
 
 	/* Call the 'apply_finish' callbacks, sorted by their priorities. */
 	RB_FOREACH (cb, nb_config_cbs, &cbs)
-		nb_callback_apply_finish(transaction->context, cb->nb_node,
+		nb_callback_apply_finish(&transaction->context, cb->nb_node,
 					 cb->dnode, errmsg, errmsg_len);
 
 	/* Release memory. */
diff --git a/lib/northbound.h b/lib/northbound.h
index c132daebd..152810b3a 100644
--- a/lib/northbound.h
+++ b/lib/northbound.h
@@ -622,22 +622,6 @@ struct nb_context {
 
 	/* Northbound user (can be NULL). */
 	const void *user;
-
-	/* Client-specific data. */
-#if 0
-	union {
-		struct {
-		} cli;
-		struct {
-		} confd;
-		struct {
-		} sysrepo;
-		struct {
-		} grpc;
-		struct {
-		} pcep;
-	} client_data;
-#endif
 };
 
 /* Northbound configuration. */
@@ -666,7 +650,7 @@ struct nb_config_change {
 
 /* Northbound configuration transaction. */
 struct nb_transaction {
-	struct nb_context *context;
+	struct nb_context context;
 	char comment[80];
 	struct nb_config *config;
 	struct nb_config_cbs changes;
@@ -927,7 +911,7 @@ extern int nb_candidate_validate(struct nb_context *context,
  *      the candidate configuration.
  *    - NB_ERR for other errors.
  */
-extern int nb_candidate_commit_prepare(struct nb_context *context,
+extern int nb_candidate_commit_prepare(struct nb_context context,
 				       struct nb_config *candidate,
 				       const char *comment,
 				       struct nb_transaction **transaction,
@@ -1014,7 +998,7 @@ extern void nb_candidate_commit_apply(struct nb_transaction *transaction,
  *      the candidate configuration.
  *    - NB_ERR for other errors.
  */
-extern int nb_candidate_commit(struct nb_context *context,
+extern int nb_candidate_commit(struct nb_context context,
 			       struct nb_config *candidate,
 			       bool save_transaction, const char *comment,
 			       uint32_t *transaction_id, char *errmsg,
diff --git a/lib/northbound_cli.c b/lib/northbound_cli.c
index 0dfa66b37..fa5884fb7 100644
--- a/lib/northbound_cli.c
+++ b/lib/northbound_cli.c
@@ -46,7 +46,7 @@ static int nb_cli_classic_commit(struct vty *vty)
 
 	context.client = NB_CLIENT_CLI;
 	context.user = vty;
-	ret = nb_candidate_commit(&context, vty->candidate_config, true, NULL,
+	ret = nb_candidate_commit(context, vty->candidate_config, true, NULL,
 				  NULL, errmsg, sizeof(errmsg));
 	switch (ret) {
 	case NB_OK:
@@ -313,7 +313,7 @@ int nb_cli_confirmed_commit_rollback(struct vty *vty)
 	context.client = NB_CLIENT_CLI;
 	context.user = vty;
 	ret = nb_candidate_commit(
-		&context, vty->confirmed_commit_rollback, true,
+		context, vty->confirmed_commit_rollback, true,
 		"Rollback to previous configuration - confirmed commit has timed out",
 		&transaction_id, errmsg, sizeof(errmsg));
 	if (ret == NB_OK) {
@@ -394,9 +394,8 @@ static int nb_cli_commit(struct vty *vty, bool force,
 
 	context.client = NB_CLIENT_CLI;
 	context.user = vty;
-	ret = nb_candidate_commit(&context, vty->candidate_config, true,
-				  comment, &transaction_id, errmsg,
-				  sizeof(errmsg));
+	ret = nb_candidate_commit(context, vty->candidate_config, true, comment,
+				  &transaction_id, errmsg, sizeof(errmsg));
 
 	/* Map northbound return code to CLI return code. */
 	switch (ret) {
@@ -1717,7 +1716,7 @@ static int nb_cli_rollback_configuration(struct vty *vty,
 
 	context.client = NB_CLIENT_CLI;
 	context.user = vty;
-	ret = nb_candidate_commit(&context, candidate, true, comment, NULL,
+	ret = nb_candidate_commit(context, candidate, true, comment, NULL,
 				  errmsg, sizeof(errmsg));
 	nb_config_free(candidate);
 	switch (ret) {
diff --git a/lib/northbound_confd.c b/lib/northbound_confd.c
index 81ba313e8..2b57ff270 100644
--- a/lib/northbound_confd.c
+++ b/lib/northbound_confd.c
@@ -311,7 +311,7 @@ static void frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen)
 	 */
 	transaction = NULL;
 	context.client = NB_CLIENT_CONFD;
-	ret = nb_candidate_commit_prepare(&context, candidate, NULL,
+	ret = nb_candidate_commit_prepare(context, candidate, NULL,
 					  &transaction, errmsg, sizeof(errmsg));
 	if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) {
 		enum confd_errcode errcode;
diff --git a/lib/northbound_db.c b/lib/northbound_db.c
index cefcfbcf1..74abcde95 100644
--- a/lib/northbound_db.c
+++ b/lib/northbound_db.c
@@ -73,7 +73,7 @@ int nb_db_transaction_save(const struct nb_transaction *transaction,
 	if (!ss)
 		goto exit;
 
-	client_name = nb_client_name(transaction->context->client);
+	client_name = nb_client_name(transaction->context.client);
 	/*
 	 * Always record configurations in the XML format, save the default
 	 * values too, as this covers the case where defaults may change.
diff --git a/lib/northbound_grpc.cpp b/lib/northbound_grpc.cpp
index f5d59d92d..1459146ea 100644
--- a/lib/northbound_grpc.cpp
+++ b/lib/northbound_grpc.cpp
@@ -824,7 +824,7 @@ HandleUnaryCommit(UnaryRpcState<frr::CommitRequest, frr::CommitResponse> *tag)
 	case frr::CommitRequest::PREPARE:
 		grpc_debug("`-> Performing PREPARE");
 		ret = nb_candidate_commit_prepare(
-			&context, candidate->config, comment.c_str(),
+			context, candidate->config, comment.c_str(),
 			&candidate->transaction, errmsg, sizeof(errmsg));
 		break;
 	case frr::CommitRequest::ABORT:
@@ -840,7 +840,7 @@ HandleUnaryCommit(UnaryRpcState<frr::CommitRequest, frr::CommitResponse> *tag)
 		break;
 	case frr::CommitRequest::ALL:
 		grpc_debug("`-> Performing ALL");
-		ret = nb_candidate_commit(&context, candidate->config, true,
+		ret = nb_candidate_commit(context, candidate->config, true,
 					  comment.c_str(), &transaction_id,
 					  errmsg, sizeof(errmsg));
 		break;
diff --git a/lib/northbound_sysrepo.c b/lib/northbound_sysrepo.c
index 824d81a51..096414ff2 100644
--- a/lib/northbound_sysrepo.c
+++ b/lib/northbound_sysrepo.c
@@ -268,7 +268,7 @@ static int frr_sr_config_change_cb_prepare(sr_session_ctx_t *session,
 	 * Validate the configuration changes and allocate all resources
 	 * required to apply them.
 	 */
-	ret = nb_candidate_commit_prepare(&context, candidate, NULL,
+	ret = nb_candidate_commit_prepare(context, candidate, NULL,
 					  &transaction, errmsg, sizeof(errmsg));
 	if (ret != NB_OK && ret != NB_ERR_NO_CHANGES)
 		flog_warn(
diff --git a/lib/vty.c b/lib/vty.c
index 76dfe9734..c485d2957 100644
--- a/lib/vty.c
+++ b/lib/vty.c
@@ -2413,7 +2413,7 @@ static void vty_read_file(struct nb_config *config, FILE *confp)
 
 		context.client = NB_CLIENT_CLI;
 		context.user = vty;
-		ret = nb_candidate_commit(&context, vty->candidate_config, true,
+		ret = nb_candidate_commit(context, vty->candidate_config, true,
 					  "Read configuration file", NULL,
 					  errmsg, sizeof(errmsg));
 		if (ret != NB_OK && ret != NB_ERR_NO_CHANGES)