From 59d2368b0f055f28aeda8f6080d686acfa35c20b Mon Sep 17 00:00:00 2001 From: Christian Hopps Date: Tue, 8 Apr 2025 05:55:03 +0000 Subject: [PATCH] mgmtd: normalize argument order to copy(dst, src) 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 --- mgmtd/mgmt_ds.c | 19 ++++++++----------- mgmtd/mgmt_ds.h | 12 +++++------- mgmtd/mgmt_txn.c | 26 ++++++++++---------------- 3 files changed, 23 insertions(+), 34 deletions(-) diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c index dabae4afd1..e83980f97b 100644 --- a/mgmtd/mgmt_ds.c +++ b/mgmtd/mgmt_ds.c @@ -74,8 +74,7 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx, return 0; } -static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, - struct mgmt_ds_ctx *dst) +static int ds_copy(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src) { if (!src || !dst) return -1; @@ -95,8 +94,7 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src, return 0; } -static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src, - struct mgmt_ds_ctx *dst) +static int ds_merge(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src) { int ret; @@ -251,14 +249,13 @@ void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx) ds_ctx->locked = 0; } -int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, - struct mgmt_ds_ctx *dst_ds_ctx, bool updt_cmt_rec) +int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, 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; - if (updt_cmt_rec && dst_ds_ctx->ds_id == MGMTD_DS_RUNNING) - mgmt_history_new_record(dst_ds_ctx); + if (updt_cmt_rec && dst->ds_id == MGMTD_DS_RUNNING) + mgmt_history_new_record(dst); return 0; } @@ -416,9 +413,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst, parsed.ds_id = dst->ds_id; if (merge) - mgmt_ds_merge_src_with_dst_ds(&parsed, dst); + ds_merge(dst, &parsed); else - mgmt_ds_replace_dst_with_src_ds(&parsed, dst); + ds_copy(dst, &parsed); nb_config_free(parsed.root.cfg_root); diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h index b8e77e330a..f7e1d7c5ee 100644 --- a/mgmtd/mgmt_ds.h +++ b/mgmtd/mgmt_ds.h @@ -196,21 +196,19 @@ extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx); /* * Copy from source to destination datastore. * - * src_ds - * Source datastore handle (ds to be copied from). - * - * dst_ds + * dst * Destination datastore handle (ds to be copied to). * + * src + * Source datastore handle (ds to be copied from). + * * update_cmd_rec * TRUE if need to update commit record, FALSE otherwise. * * Returns: * 0 on success, -1 on failure. */ -extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx, - struct mgmt_ds_ctx *dst_ds_ctx, - bool update_cmt_rec); +extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool update_cmt_rec); /* * Fetch northbound configuration for a given datastore context. diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 483dfab8e8..2b4734e971 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -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); /* - * 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. */ if ((txn->session_id && !txn->commit_cfg_req->req.commit_cfg.validate_only && !txn->commit_cfg_req->req.commit_cfg.abort) || txn->commit_cfg_req->req.commit_cfg.rollback) { - mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg - .src_ds_ctx, - txn->commit_cfg_req->req.commit_cfg - .dst_ds_ctx, + mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, + txn->commit_cfg_req->req.commit_cfg.src_ds_ctx, create_cmt_info_rec); } @@ -783,22 +781,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn, * request. */ if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort) - mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg - .dst_ds_ctx, - txn->commit_cfg_req->req.commit_cfg - .src_ds_ctx, - false); + mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx, + txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false); } else { /* * 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) - mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg - .dst_ds_ctx, - txn->commit_cfg_req->req.commit_cfg - .src_ds_ctx, - false); + mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx, + txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false); } if (txn->commit_cfg_req->req.commit_cfg.rollback) { -- 2.39.5