Mirror/frr
0
0
Fork 1
mirror of https://github.com/FRRouting/frr.git synced 2025-04-30 13:37:17 +02:00
Code Issues Actions 4 Packages Projects Releases Wiki Activity

mgmtd: normalize argument order to copy(dst, src)

Browse source 
Having just completed a code audit during RCA, the fact that we have 2
different argument orders for the related datastore copying functions
was unnecessary and super confusing.

Fix this code-maintenance/comprehension mistake and move the newer mgmtd
copy routines to use the same arg order as the pre-existing underlying
northbound copy functions (i.e., use `copy(dst, src)`)

Signed-off-by: Christian Hopps <chopps@labn.net>
This commit is contained in:
Christian Hopps 2025-04-08 05:55:03 +00:00
parent b12b4c28b4
commit 59d2368b0f
3 changed files with 23 additions and 34 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
mgmtd/mgmt_ds.c
View file

@ -74,8 +74,7 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx,
return 0; return 0;
} }
static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, static int ds_copy(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
struct mgmt_ds_ctx *dst)
{ {
if (!src || !dst) if (!src || !dst)
return -1; return -1;
@ -95,8 +94,7 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
return 0; return 0;
} }
static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, static int ds_merge(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
struct mgmt_ds_ctx *dst)
{ {
int ret; int ret;
@ -251,14 +249,13 @@ void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx)
ds_ctx->locked = 0; ds_ctx->locked = 0;
} }
int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool updt_cmt_rec)
struct mgmt_ds_ctx *dst_ds_ctx, bool updt_cmt_rec)
{ {
if (mgmt_ds_replace_dst_with_src_ds(src_ds_ctx, dst_ds_ctx) != 0) if (ds_copy(dst, src) != 0)
return -1; return -1;
if (updt_cmt_rec && dst_ds_ctx->ds_id == MGMTD_DS_RUNNING) if (updt_cmt_rec && dst->ds_id == MGMTD_DS_RUNNING)
mgmt_history_new_record(dst_ds_ctx); mgmt_history_new_record(dst);
return 0; return 0;
} }
@ -416,9 +413,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst,
parsed.ds_id = dst->ds_id; parsed.ds_id = dst->ds_id;
if (merge) if (merge)
mgmt_ds_merge_src_with_dst_ds(&parsed, dst); ds_merge(dst, &parsed);
else else
mgmt_ds_replace_dst_with_src_ds(&parsed, dst); ds_copy(dst, &parsed);
nb_config_free(parsed.root.cfg_root); nb_config_free(parsed.root.cfg_root);

12
mgmtd/mgmt_ds.h
View file

@ -196,21 +196,19 @@ extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx);
/* /*
* Copy from source to destination datastore. * Copy from source to destination datastore.
* *
* src_ds * dst
* Source datastore handle (ds to be copied from).
*
* dst_ds
* Destination datastore handle (ds to be copied to). * Destination datastore handle (ds to be copied to).
* *
* src
* Source datastore handle (ds to be copied from).
*
* update_cmd_rec * update_cmd_rec
* TRUE if need to update commit record, FALSE otherwise. * TRUE if need to update commit record, FALSE otherwise.
* *
* Returns: * Returns:
* 0 on success, -1 on failure. * 0 on success, -1 on failure.
*/ */
extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool update_cmt_rec);
struct mgmt_ds_ctx *dst_ds_ctx,
bool update_cmt_rec);
/* /*
* Fetch northbound configuration for a given datastore context. * Fetch northbound configuration for a given datastore context.

26
mgmtd/mgmt_txn.c
View file

@ -764,17 +764,15 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
!txn->commit_cfg_req->req.commit_cfg.rollback); !txn->commit_cfg_req->req.commit_cfg.rollback);
/* /*
* Successful commit: Merge Src DS into Dst DS if and only if * Successful commit: Copy Src DS to Dst DS if and only if
* this was not a validate-only or abort request. * this was not a validate-only or abort request.
*/ */
if ((txn->session_id && if ((txn->session_id &&
!txn->commit_cfg_req->req.commit_cfg.validate_only && !txn->commit_cfg_req->req.commit_cfg.validate_only &&
!txn->commit_cfg_req->req.commit_cfg.abort) || !txn->commit_cfg_req->req.commit_cfg.abort) ||
txn->commit_cfg_req->req.commit_cfg.rollback) { txn->commit_cfg_req->req.commit_cfg.rollback) {
mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx,
.src_ds_ctx, txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
txn->commit_cfg_req->req.commit_cfg
.dst_ds_ctx,
create_cmt_info_rec); create_cmt_info_rec);
} }
@ -783,22 +781,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
* request. * request.
*/ */
if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort) if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort)
mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
.dst_ds_ctx, txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
txn->commit_cfg_req->req.commit_cfg
.src_ds_ctx,
false);
} else { } else {
/* /*
* The commit has failied. For implicit commit requests restore * The commit has failied. For implicit commit requests restore
* back the contents of the candidate DS. * back the contents of the candidate DS. For non-implicit
* commit we want to allow the user to re-commit on the changes
* (whether further modified or not).
*/ */
if (txn->commit_cfg_req->req.commit_cfg.implicit) if (txn->commit_cfg_req->req.commit_cfg.implicit)
mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
.dst_ds_ctx, txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
txn->commit_cfg_req->req.commit_cfg
.src_ds_ctx,
false);
} }
if (txn->commit_cfg_req->req.commit_cfg.rollback) { if (txn->commit_cfg_req->req.commit_cfg.rollback) {