proxmox-mirrors/mirror_frr
1
0
Fork 0
mirror of https://git.proxmox.com/git/mirror_frr synced 2025-04-29 10:11:52 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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>
This commit is contained in:
Christian Hopps 2023-02-23 20:23:51 -05:00
parent ce8194bcd0
commit 41ef7327e3
9 changed files with 24 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
lib/libfrr.c
View File

@ -992,7 +992,7 @@ static void frr_config_read_in(struct thread *t)
int ret; int ret;
context.client = NB_CLIENT_CLI; 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, true, "Read configuration file", NULL,
errmsg, sizeof(errmsg)); errmsg, sizeof(errmsg));
if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) if (ret != NB_OK && ret != NB_ERR_NO_CHANGES)

19
lib/northbound.c
View File

@ -61,7 +61,7 @@ static int nb_callback_configuration(struct nb_context *context,
struct nb_config_change *change, struct nb_config_change *change,
char *errmsg, size_t errmsg_len); char *errmsg, size_t errmsg_len);
static struct nb_transaction * 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, struct nb_config_cbs *changes, const char *comment,
char *errmsg, size_t errmsg_len); char *errmsg, size_t errmsg_len);
static void nb_transaction_free(struct nb_transaction *transaction); static void nb_transaction_free(struct nb_transaction *transaction);
@ -835,7 +835,7 @@ int nb_candidate_validate(struct nb_context *context,
return ret; return ret;
} }
int nb_candidate_commit_prepare(struct nb_context *context, int nb_candidate_commit_prepare(struct nb_context context,
struct nb_config *candidate, struct nb_config *candidate,
const char *comment, const char *comment,
struct nb_transaction **transaction, struct nb_transaction **transaction,
@ -860,9 +860,8 @@ int nb_candidate_commit_prepare(struct nb_context *context,
return NB_ERR_NO_CHANGES; return NB_ERR_NO_CHANGES;
} }
if (nb_candidate_validate_code(context, candidate, &changes, errmsg, if (nb_candidate_validate_code(&context, candidate, &changes, errmsg,
errmsg_len) errmsg_len) != NB_OK) {
!= NB_OK) {
flog_warn(EC_LIB_NB_CANDIDATE_INVALID, flog_warn(EC_LIB_NB_CANDIDATE_INVALID,
"%s: failed to validate candidate configuration", "%s: failed to validate candidate configuration",
__func__); __func__);
@ -913,7 +912,7 @@ void nb_candidate_commit_apply(struct nb_transaction *transaction,
nb_transaction_free(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, bool save_transaction, const char *comment,
uint32_t *transaction_id, char *errmsg, uint32_t *transaction_id, char *errmsg,
size_t errmsg_len) size_t errmsg_len)
@ -1411,13 +1410,13 @@ static int nb_callback_configuration(struct nb_context *context,
} }
static struct nb_transaction * 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, struct nb_config_cbs *changes, const char *comment,
char *errmsg, size_t errmsg_len) char *errmsg, size_t errmsg_len)
{ {
struct nb_transaction *transaction; struct nb_transaction *transaction;
if (nb_running_lock_check(context->client, context->user)) { if (nb_running_lock_check(context.client, context.user)) {
strlcpy(errmsg, strlcpy(errmsg,
"running configuration is locked by another client", "running configuration is locked by another client",
errmsg_len); errmsg_len);
@ -1469,7 +1468,7 @@ static int nb_transaction_process(enum nb_event event,
break; break;
/* Call the appropriate callback. */ /* Call the appropriate callback. */
ret = nb_callback_configuration(transaction->context, event, ret = nb_callback_configuration(&transaction->context, event,
change, errmsg, errmsg_len); change, errmsg, errmsg_len);
switch (event) { switch (event) {
case NB_EV_PREPARE: 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. */ /* Call the 'apply_finish' callbacks, sorted by their priorities. */
RB_FOREACH (cb, nb_config_cbs, &cbs) 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); cb->dnode, errmsg, errmsg_len);
/* Release memory. */ /* Release memory. */

22
lib/northbound.h
View File

@ -622,22 +622,6 @@ struct nb_context {
/* Northbound user (can be NULL). */ /* Northbound user (can be NULL). */
const void *user; const void *user;
/* Client-specific data. */
#if 0
union {
struct {
} cli;
struct {
} confd;
struct {
} sysrepo;
struct {
} grpc;
struct {
} pcep;
} client_data;
#endif
}; };
/* Northbound configuration. */ /* Northbound configuration. */
@ -666,7 +650,7 @@ struct nb_config_change {
/* Northbound configuration transaction. */ /* Northbound configuration transaction. */
struct nb_transaction { struct nb_transaction {
struct nb_context *context; struct nb_context context;
char comment[80]; char comment[80];
struct nb_config *config; struct nb_config *config;
struct nb_config_cbs changes; struct nb_config_cbs changes;
@ -927,7 +911,7 @@ extern int nb_candidate_validate(struct nb_context *context,
* the candidate configuration. * the candidate configuration.
* - NB_ERR for other errors. * - 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, struct nb_config *candidate,
const char *comment, const char *comment,
struct nb_transaction **transaction, struct nb_transaction **transaction,
@ -1014,7 +998,7 @@ extern void nb_candidate_commit_apply(struct nb_transaction *transaction,
* the candidate configuration. * the candidate configuration.
* - NB_ERR for other errors. * - 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, struct nb_config *candidate,
bool save_transaction, const char *comment, bool save_transaction, const char *comment,
uint32_t *transaction_id, char *errmsg, uint32_t *transaction_id, char *errmsg,

11
lib/northbound_cli.c
View File

@ -46,7 +46,7 @@ static int nb_cli_classic_commit(struct vty *vty)
context.client = NB_CLIENT_CLI; context.client = NB_CLIENT_CLI;
context.user = vty; 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)); NULL, errmsg, sizeof(errmsg));
switch (ret) { switch (ret) {
case NB_OK: case NB_OK:
@ -313,7 +313,7 @@ int nb_cli_confirmed_commit_rollback(struct vty *vty)
context.client = NB_CLIENT_CLI; context.client = NB_CLIENT_CLI;
context.user = vty; context.user = vty;
ret = nb_candidate_commit( 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", "Rollback to previous configuration - confirmed commit has timed out",
&transaction_id, errmsg, sizeof(errmsg)); &transaction_id, errmsg, sizeof(errmsg));
if (ret == NB_OK) { if (ret == NB_OK) {
@ -394,9 +394,8 @@ static int nb_cli_commit(struct vty *vty, bool force,
context.client = NB_CLIENT_CLI; context.client = NB_CLIENT_CLI;
context.user = vty; context.user = vty;
ret = nb_candidate_commit(&context, vty->candidate_config, true, ret = nb_candidate_commit(context, vty->candidate_config, true, comment,
comment, &transaction_id, errmsg, &transaction_id, errmsg, sizeof(errmsg));
sizeof(errmsg));
/* Map northbound return code to CLI return code. */ /* Map northbound return code to CLI return code. */
switch (ret) { switch (ret) {
@ -1717,7 +1716,7 @@ static int nb_cli_rollback_configuration(struct vty *vty,
context.client = NB_CLIENT_CLI; context.client = NB_CLIENT_CLI;
context.user = vty; context.user = vty;
ret = nb_candidate_commit(&context, candidate, true, comment, NULL, ret = nb_candidate_commit(context, candidate, true, comment, NULL,
errmsg, sizeof(errmsg)); errmsg, sizeof(errmsg));
nb_config_free(candidate); nb_config_free(candidate);
switch (ret) { switch (ret) {

2
lib/northbound_confd.c
View File

@ -311,7 +311,7 @@ static void frr_confd_cdb_read_cb_prepare(int fd, int *subp, int reslen)
*/ */
transaction = NULL; transaction = NULL;
context.client = NB_CLIENT_CONFD; 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)); &transaction, errmsg, sizeof(errmsg));
if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) { if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) {
enum confd_errcode errcode; enum confd_errcode errcode;

2
lib/northbound_db.c
View File

@ -73,7 +73,7 @@ int nb_db_transaction_save(const struct nb_transaction *transaction,
if (!ss) if (!ss)
goto exit; 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 * Always record configurations in the XML format, save the default
* values too, as this covers the case where defaults may change. * values too, as this covers the case where defaults may change.

4
lib/northbound_grpc.cpp
View File

@ -824,7 +824,7 @@ HandleUnaryCommit(UnaryRpcState<frr::CommitRequest, frr::CommitResponse> *tag)
case frr::CommitRequest::PREPARE: case frr::CommitRequest::PREPARE:
grpc_debug("`-> Performing PREPARE"); grpc_debug("`-> Performing PREPARE");
ret = nb_candidate_commit_prepare( ret = nb_candidate_commit_prepare(
&context, candidate->config, comment.c_str(), context, candidate->config, comment.c_str(),
&candidate->transaction, errmsg, sizeof(errmsg)); &candidate->transaction, errmsg, sizeof(errmsg));
break; break;
case frr::CommitRequest::ABORT: case frr::CommitRequest::ABORT:
@ -840,7 +840,7 @@ HandleUnaryCommit(UnaryRpcState<frr::CommitRequest, frr::CommitResponse> *tag)
break; break;
case frr::CommitRequest::ALL: case frr::CommitRequest::ALL:
grpc_debug("`-> Performing 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, comment.c_str(), &transaction_id,
errmsg, sizeof(errmsg)); errmsg, sizeof(errmsg));
break; break;

2
lib/northbound_sysrepo.c
View File

@ -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 * Validate the configuration changes and allocate all resources
* required to apply them. * required to apply them.
*/ */
ret = nb_candidate_commit_prepare(&context, candidate, NULL, ret = nb_candidate_commit_prepare(context, candidate, NULL,
&transaction, errmsg, sizeof(errmsg)); &transaction, errmsg, sizeof(errmsg));
if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) if (ret != NB_OK && ret != NB_ERR_NO_CHANGES)
flog_warn( flog_warn(

2
lib/vty.c
View File

@ -2413,7 +2413,7 @@ static void vty_read_file(struct nb_config *config, FILE *confp)
context.client = NB_CLIENT_CLI; context.client = NB_CLIENT_CLI;
context.user = vty; 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, "Read configuration file", NULL,
errmsg, sizeof(errmsg)); errmsg, sizeof(errmsg));
if (ret != NB_OK && ret != NB_ERR_NO_CHANGES) if (ret != NB_OK && ret != NB_ERR_NO_CHANGES)