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>

diff --git a/lib/libfrr.c b/lib/libfrr.c
index 0467dc1d7e211fbe8c9f97d96ce70586d4c0db63..d1b7dd133e61ee3ed3c1fb92f6aa3bcb1a3cd558 100644 (file)
--- 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 b755264be19526087fc730fe1d1ff7ffb623d1e0..6f2c522a294ff016074f7ce1dafed8fd9c2e5316 100644 (file)
--- 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 c132daebdb3834a9f1b5ecc356c02c584a79b4e1..152810b3a98221db90a2b10eb0a530fad06d876d 100644 (file)
--- 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 0dfa66b37e5dd030232264554a9bd34ab01723c4..fa5884fb7850c49d21a6966d2b65f2ec627a0cfc 100644 (file)
--- 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 81ba313e810afe96660d68ca1019d83da7aa9141..2b57ff27070f69f129df0584a4fce11ddd606578 100644 (file)
--- 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 cefcfbcf1f99d68ba90d3b8b998705b8083dab37..74abcde955a9e3c49971087c05d0c5cb301ec6b1 100644 (file)
--- 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 f5d59d92d6cd63f858523ab4a15bb03cc26a7b6e..1459146eab1c82c46f15829b2b3eca424e9e0af8 100644 (file)
--- 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 824d81a51e651ad73189bf1f17dc2f69446b4cff..096414ff2474c7846a0d3a3a7be30833e9229ace 100644 (file)
--- 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 76dfe9734e684edbd011417e07dc4c999d61692b..c485d2957979536946755ccd776719c8e7ff1dfd 100644 (file)
--- 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)