From 2e21fba657ba4ed5f9ff380756c437bcc5d64c1a Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 10:07:30 +0000 Subject: [PATCH 1/9] DAOS-18304 ddb: store VOS path in vos_file_parts and raise path size limits - Add vf_vos_file_path field to vos_file_parts to retain the original VOS file path through the parsing pipeline, so callers no longer need to keep the input pointer alive after parse_vos_file_parts() returns. - Rename the enum to introduce VOS_PATH_SIZE (alongside DB_PATH_SIZE) and raise both constants from 256 to PATH_MAX. - Validate the VOS path length in parse_vos_file_parts() and return -DER_EXCEEDS_PATH_LEN when the limit is exceeded. Signed-off-by: Cedric Koch-Hofer --- src/utils/ddb/ddb_parse.c | 10 ++++++++++ src/utils/ddb/ddb_parse.h | 7 ++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/utils/ddb/ddb_parse.c b/src/utils/ddb/ddb_parse.c index 67950a90ab2..5e5946fd400 100644 --- a/src/utils/ddb/ddb_parse.c +++ b/src/utils/ddb/ddb_parse.c @@ -195,6 +195,7 @@ parse_vos_file_parts(const char *vos_path, const char *db_path, regex_t preg; regmatch_t match[MATCH_SIZE]; struct vos_file_parts *vfp_tmp; + size_t vos_path_len; int rc; D_ASSERT(vos_path != NULL && vos_file_parts != NULL); @@ -217,6 +218,15 @@ parse_vos_file_parts(const char *vos_path, const char *db_path, } D_ASSERT(rc == 0); + vos_path_len = strnlen(vos_path, VOS_PATH_SIZE); + if (vos_path_len >= VOS_PATH_SIZE) { + D_ERROR("VOS path '%s' too long: got=%zu, max=%d\n", vos_path, vos_path_len, + VOS_PATH_SIZE - 1); + rc = -DER_EXCEEDS_PATH_LEN; + goto out_preg; + } + memcpy(vfp_tmp->vf_vos_file_path, vos_path, vos_path_len + 1); + if (db_path != NULL && db_path[0] != '\0') { size_t db_path_len = strnlen(db_path, PATH_MAX); if (db_path_len >= DB_PATH_SIZE) { diff --git a/src/utils/ddb/ddb_parse.h b/src/utils/ddb/ddb_parse.h index 15937d0a562..c7da5fda375 100644 --- a/src/utils/ddb/ddb_parse.h +++ b/src/utils/ddb/ddb_parse.h @@ -15,8 +15,9 @@ #include #include "ddb_common.h" -enum { DB_PATH_SIZE = 256, VOS_FILE_NAME_SIZE = 16 }; +enum { VOS_PATH_SIZE = PATH_MAX, DB_PATH_SIZE = PATH_MAX, VOS_FILE_NAME_SIZE = 16 }; struct vos_file_parts { + char vf_vos_file_path[VOS_PATH_SIZE]; char vf_db_path[DB_PATH_SIZE]; uuid_t vf_pool_uuid; char vf_vos_file_name[VOS_FILE_NAME_SIZE]; @@ -25,8 +26,8 @@ struct vos_file_parts { /* Parse a path to a VOS file to get needed parts for initializing vos */ int - parse_vos_file_parts(const char *vos_path, const char *db_path, - struct vos_file_parts *vos_file_parts); + parse_vos_file_parts(const char *vos_path, const char *db_path, + struct vos_file_parts *vos_file_parts); /* See ddb_key_to_printable_buf for how the keys will be printed */ int ddb_parse_key(const char *input, daos_key_t *key); From dfc555ef0bbca0577f63f410fd5b805099b28640 Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 10:07:46 +0000 Subject: [PATCH 2/9] DAOS-18304 ddb: simplify dv_pool_open/destroy API and remove path caching from ddb_ctx Remove dc_pool_path and dc_db_path from ddb_ctx: callers now pass paths as explicit arguments instead of storing them in the context, eliminating the need for the SetCString() helper. dv_pool_open() and dv_pool_destroy() signatures are simplified from (path, path_parts, poh, flags, write_mode) to (path, db_path, ctx, flags): - vos_file_parts allocation is moved into the new internal helper create_vos_file_parts(), keeping the API surface minimal. - dc_write_mode and dc_poh are set directly on the ctx, so the caller gets the pool handle back without an extra out-parameter. ddb_run_open(), ddb_run_feature() and ddb_run_rm_pool() in ddb_commands.c are simplified accordingly. ddb_run_feature() also gains an explicit error message on dv_pool_open() failure. ddb_run_dtx_stat() no longer prints ctx->dc_pool_path (which no longer exists). Signed-off-by: Cedric Koch-Hofer --- src/utils/ddb/ddb.h | 8 +-- src/utils/ddb/ddb_commands.c | 46 +++------------ src/utils/ddb/ddb_vos.c | 105 ++++++++++++++++++++++++++--------- src/utils/ddb/ddb_vos.h | 8 +-- 4 files changed, 93 insertions(+), 74 deletions(-) diff --git a/src/utils/ddb/ddb.h b/src/utils/ddb/ddb.h index c079b25632b..7107dcc4dc3 100644 --- a/src/utils/ddb/ddb.h +++ b/src/utils/ddb/ddb.h @@ -64,11 +64,9 @@ struct ddb_io_ft { }; struct ddb_ctx { - struct ddb_io_ft dc_io_ft; - daos_handle_t dc_poh; - bool dc_write_mode; - const char *dc_pool_path; - const char *dc_db_path; + struct ddb_io_ft dc_io_ft; + daos_handle_t dc_poh; + bool dc_write_mode; }; void ddb_ctx_init(struct ddb_ctx *ctx); diff --git a/src/utils/ddb/ddb_commands.c b/src/utils/ddb/ddb_commands.c index 63af1e1f522..2e2e7a4c159 100644 --- a/src/utils/ddb/ddb_commands.c +++ b/src/utils/ddb/ddb_commands.c @@ -73,20 +73,10 @@ ddb_pool_is_open(struct ddb_ctx *ctx) int ddb_run_open(struct ddb_ctx *ctx, struct open_options *opt) { - struct vos_file_parts path_parts = {0}; - int rc; - DDB_POOL_SHOULD_CLOSE(ctx); - rc = parse_vos_file_parts(opt->path, opt->db_path, &path_parts); - if (!SUCCESS(rc)) - return rc; - - DDB_CAN_PROCEED(ctx, path_parts.vf_db_path); - ctx->dc_write_mode = opt->write_mode; - - return dv_pool_open(opt->path, &path_parts, &ctx->dc_poh, 0, ctx->dc_write_mode); + return dv_pool_open(opt->path, opt->db_path, ctx, 0); } int @@ -1082,7 +1072,6 @@ feature_write_action(struct feature_options *opt) int ddb_run_feature(struct ddb_ctx *ctx, struct feature_options *opt) { - struct vos_file_parts path_parts = {0}; int rc; uint64_t new_compat_flags; uint64_t new_incompat_flags; @@ -1101,22 +1090,11 @@ ddb_run_feature(struct ddb_ctx *ctx, struct feature_options *opt) if (feature_write_action(opt) && !ctx->dc_write_mode) return -DER_NO_PERM; - if (!opt->path || strnlen(opt->path, PATH_MAX) == 0) - opt->path = ctx->dc_pool_path; - - if (!opt->db_path || strnlen(opt->db_path, PATH_MAX) == 0) - opt->db_path = ctx->dc_db_path; - - rc = parse_vos_file_parts(opt->path, opt->db_path, &path_parts); - if (!SUCCESS(rc)) - return rc; - - DDB_CAN_PROCEED(ctx, path_parts.vf_db_path); - - rc = dv_pool_open(opt->path, &path_parts, &ctx->dc_poh, VOS_POF_FOR_FEATURE_FLAG, - ctx->dc_write_mode); - if (rc) + rc = dv_pool_open(opt->path, opt->db_path, ctx, VOS_POF_FOR_FEATURE_FLAG); + if (rc) { + ddb_errorf(ctx, "Unable to open VOS pool '%s'\n", opt->path); return rc; + } close = true; skip: @@ -1152,25 +1130,15 @@ ddb_run_feature(struct ddb_ctx *ctx, struct feature_options *opt) rc = dv_pool_close(ctx->dc_poh); ctx->dc_poh = DAOS_HDL_INVAL; ctx->dc_write_mode = false; - return rc; } int ddb_run_rm_pool(struct ddb_ctx *ctx, struct rm_pool_options *opt) { - struct vos_file_parts path_parts = {0}; - int rc; - DDB_POOL_SHOULD_CLOSE(ctx); - rc = parse_vos_file_parts(opt->path, opt->db_path, &path_parts); - if (!SUCCESS(rc)) - return rc; - - DDB_CAN_PROCEED(ctx, path_parts.vf_db_path); - - return dv_pool_destroy(opt->path, &path_parts); + return dv_pool_destroy(opt->path, opt->db_path, ctx); } #define DTI_ALL "all" @@ -1659,7 +1627,7 @@ ddb_run_dtx_stat(struct ddb_ctx *ctx, struct dtx_stat_options *opt) rc = vos_iterate(¶m, VOS_ITER_COUUID, false, &anchors, NULL, dtx_stat_cont_cb, &args, NULL); } while (rc > 0); - ddb_printf(ctx, "DTX entries statistics of the pool %s\n", ctx->dc_pool_path); + ddb_print(ctx, "DTX entries statistics of the pool:\n"); if (opt->details) rc = dtx_stat_print(ctx, args.cmt_cnt, &args.time_stat, args.aggr_epoch); else diff --git a/src/utils/ddb/ddb_vos.c b/src/utils/ddb/ddb_vos.c index 65f567a39e9..9aebb27b021 100644 --- a/src/utils/ddb/ddb_vos.c +++ b/src/utils/ddb/ddb_vos.c @@ -27,12 +27,49 @@ vos_iterate(param, iter_type, recursive, \ anchors, cb, NULL, args, NULL) +static int +create_vos_file_parts(const char *path, const char *db_path, struct vos_file_parts **vf_ptr) +{ + struct vos_file_parts *vf; + int rc; + + D_ALLOC_PTR(vf); + if (vf == NULL) { + D_ERROR("Unable to allocate memory for pool path\n"); + rc = -DER_NOMEM; + goto out; + } + + rc = parse_vos_file_parts(path, db_path, vf); + if (SUCCESS(rc)) { + *vf_ptr = vf; + } else { + D_ERROR("Unable to parse VOS pool path '%s' and DB path '%s'\n", path, + db_path ? db_path : "(null)"); + D_FREE(vf); + } + +out: + return rc; +} + +bool +vmd_wa_can_proceed(struct ddb_ctx *ctx, const char *db_path); + int -dv_pool_open(const char *path, struct vos_file_parts *path_parts, daos_handle_t *poh, - uint32_t flags, bool write_mode) +dv_pool_open(const char *path, const char *db_path, struct ddb_ctx *ctx, uint32_t flags) { - int cow_val; - int rc; + int rc; + struct vos_file_parts *vf; + + rc = create_vos_file_parts(path, db_path, &vf); + if (!SUCCESS(rc)) + goto out; + + if (!vmd_wa_can_proceed(ctx, vf->vf_db_path)) { + rc = -DER_NO_SERVICE; + goto out_vf; + } /** * When the user requests read‑only mode (write_mode == false), DDB itself will not attempt @@ -49,58 +86,74 @@ dv_pool_open(const char *path, struct vos_file_parts *path_parts, daos_handle_t * modified, a new private copy is allocated. As a result, any changes made to * the mapped memory do not propagate to the persistent medium. */ - if (!write_mode) { - cow_val = 1; - rc = pmemobj_ctl_set(NULL, "copy_on_write.at_open", &cow_val); + if (!ctx->dc_write_mode) { + int cow_val = 1; + rc = pmemobj_ctl_set(NULL, "copy_on_write.at_open", &cow_val); if (rc != 0) { - return daos_errno2der(errno); + rc = daos_errno2der(errno); + goto out_vf; } } - rc = vos_self_init(path_parts->vf_db_path, true, path_parts->vf_target_idx); + rc = vos_self_init(vf->vf_db_path, true, vf->vf_target_idx); if (!SUCCESS(rc)) { - D_ERROR("Failed to initialize VOS with path '%s': " DF_RC "\n", - path_parts->vf_db_path, DP_RC(rc)); - goto exit; + D_ERROR("Failed to initialize VOS with DB path '%s': " DF_RC "\n", vf->vf_db_path, + DP_RC(rc)); + goto out_cow; } - rc = vos_pool_open(path, path_parts->vf_pool_uuid, flags, poh); + rc = vos_pool_open(vf->vf_vos_file_path, vf->vf_pool_uuid, flags, &ctx->dc_poh); if (!SUCCESS(rc)) { D_ERROR("Failed to open pool: "DF_RC"\n", DP_RC(rc)); vos_self_fini(); } -exit: - if (!write_mode) { +out_cow: + if (!ctx->dc_write_mode) { /** Restore the default value. */ - cow_val = 0; + int cow_val = 0; pmemobj_ctl_set(NULL, "copy_on_write.at_open", &cow_val); } - +out_vf: + D_FREE(vf); +out: return rc; } int -dv_pool_destroy(const char *path, struct vos_file_parts *path_parts) +dv_pool_destroy(const char *path, const char *db_path, struct ddb_ctx *ctx) { - int rc, flags = 0; + struct vos_file_parts *vf; + int flags = 0; + int rc; - rc = vos_self_init(path_parts->vf_db_path, true, path_parts->vf_target_idx); + rc = create_vos_file_parts(path, db_path, &vf); + if (!SUCCESS(rc)) + goto out; + + if (!vmd_wa_can_proceed(ctx, vf->vf_db_path)) { + rc = -DER_NO_SERVICE; + goto out_vf; + } + + rc = vos_self_init(vf->vf_db_path, true, vf->vf_target_idx); if (!SUCCESS(rc)) { - D_ERROR("Failed to initialize VOS with path '%s': " DF_RC "\n", - path_parts->vf_db_path, DP_RC(rc)); - return rc; + D_ERROR("Failed to initialize VOS with DB path '%s': " DF_RC "\n", vf->vf_db_path, + DP_RC(rc)); + goto out_vf; } - if (strncmp(path_parts->vf_vos_file_name, "rdb", 3) == 0) + if (strncmp(vf->vf_vos_file_name, "rdb", 3) == 0) flags |= VOS_POF_RDB; - rc = vos_pool_destroy_ex(path, path_parts->vf_pool_uuid, flags); + rc = vos_pool_destroy_ex(vf->vf_vos_file_path, vf->vf_pool_uuid, flags); if (!SUCCESS(rc)) D_ERROR("Failed to destroy pool: " DF_RC "\n", DP_RC(rc)); - vos_self_fini(); +out_vf: + D_FREE(vf); +out: return rc; } diff --git a/src/utils/ddb/ddb_vos.h b/src/utils/ddb/ddb_vos.h index 68740540cf6..06fef829895 100644 --- a/src/utils/ddb/ddb_vos.h +++ b/src/utils/ddb/ddb_vos.h @@ -53,11 +53,11 @@ struct ddb_array { /* Open and close a pool for a ddb_ctx */ int - dv_pool_open(const char *path, struct vos_file_parts *path_parts, daos_handle_t *poh, - uint32_t flags, bool write_mode); -int dv_pool_close(daos_handle_t poh); +dv_pool_open(const char *path, const char *db_path, struct ddb_ctx *ctx, uint32_t flags); int -dv_pool_destroy(const char *path, struct vos_file_parts *path_parts); +dv_pool_close(daos_handle_t poh); +int +dv_pool_destroy(const char *path, const char *db_path, struct ddb_ctx *ctx); /* Update vos pool flags */ int From 03378bb5a84a4f3969f15239379d74d4d638c57a Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 10:08:06 +0000 Subject: [PATCH 3/9] DAOS-18304 ddb: update C test suites for new dv_pool_open/destroy API Update all dv_pool_open() and dv_pool_destroy() call sites to use the new (path, db_path, ctx) signature: path_parts is no longer allocated by callers, and the pool handle is read back from ctx->dc_poh. ddb_parse_tests.c: add vf_vos_file_path assertions in success cases; add a test for VOS paths exceeding VOS_PATH_SIZE; add a dedicated MD-on-SSD test for DB_PATH_SIZE independently of VOS_PATH_SIZE; introduce the MOCKED_VOS_PATH_STR macro to avoid repeated string literals. ddb_commands_tests.c: move dvt_vos_insert_2_records_with_dtx() into dcv_suit_setup() so dtx records are present for all tests in the suite; correct VOS tree path indices for ls, value_dump and ilog_dump tests; update the dtx_stat expected message. ddb_vos_tests.c: update open_pool_test(), pool_flags_tests(), dv_test_setup() and helper_stat_open_modify_close_stat() for the new API. Signed-off-by: Cedric Koch-Hofer --- src/utils/ddb/tests/ddb_commands_tests.c | 31 +++++++------- src/utils/ddb/tests/ddb_parse_tests.c | 47 +++++++++++++++------ src/utils/ddb/tests/ddb_test_driver.c | 7 ++-- src/utils/ddb/tests/ddb_vos_tests.c | 52 +++++++++++------------- 4 files changed, 77 insertions(+), 60 deletions(-) diff --git a/src/utils/ddb/tests/ddb_commands_tests.c b/src/utils/ddb/tests/ddb_commands_tests.c index da90302ef3c..d0849819442 100644 --- a/src/utils/ddb/tests/ddb_commands_tests.c +++ b/src/utils/ddb/tests/ddb_commands_tests.c @@ -89,7 +89,7 @@ ls_cmd_tests(void **state) /* printing a recx works */ dvt_fake_print_called = 0; - opt.path = "/[0]/[0]/[0]/[0]/[0]"; + opt.path = "/[0]/[0]/[0]/[1]/[0]"; opt.recursive = true; assert_success(ddb_run_ls(&ctx, &opt)); @@ -142,7 +142,7 @@ dump_value_cmd_tests(void **state) assert_rc_equal(ddb_run_value_dump(&ctx, &opt), -DDBER_INCOMPLETE_PATH_VALUE); /* Path is complete, no destination means will dump to screen */ - opt.path = "[0]/[0]/[0]/[1]"; + opt.path = "[0]/[0]/[0]/[2]"; assert_success(ddb_run_value_dump(&ctx, &opt)); /* success */ @@ -188,7 +188,7 @@ dump_ilog_cmd_tests(void **state) assert_rc_equal(ddb_run_ilog_dump(&ctx, &opt), -DER_INVAL); /* Dump akey ilog */ - opt.path = "[0]/[0]/[0]/[0]"; + opt.path = "[0]/[0]/[0]/[1]"; assert_success(ddb_run_ilog_dump(&ctx, &opt)); } @@ -211,8 +211,7 @@ dump_dtx_cmd_tests(void **state) { struct dt_vos_pool_ctx *tctx = *state; struct ddb_ctx ctx = {0}; - struct dtx_dump_options opt = {0}; - daos_handle_t coh; + struct dtx_dump_options opt = {0}; dvt_fake_print_reset(); @@ -222,11 +221,6 @@ dump_dtx_cmd_tests(void **state) assert_invalid(ddb_run_dtx_dump(&ctx, &opt)); - assert_success(vos_cont_open(tctx->dvt_poh, g_uuids[0], &coh)); - - dvt_vos_insert_2_records_with_dtx(coh); - vos_cont_close(coh); - opt.path = "[0]"; assert_success(ddb_run_dtx_dump(&ctx, &opt)); @@ -457,8 +451,7 @@ dtx_stat_tests(void **state) buf[59] += i; assert_regex_match(dvt_fake_print_buffer, buf); } - assert_regex_match(dvt_fake_print_buffer, - "^DTX entries statistics of the pool \\(null\\)$"); + assert_regex_match(dvt_fake_print_buffer, "^DTX entries statistics of the pool:$"); } static uint64_t @@ -567,18 +560,24 @@ dtx_aggr_tests(void **state) static int dcv_suit_setup(void **state) { + struct ddb_ctx ctx = {0}; struct dt_vos_pool_ctx *tctx; - struct vos_file_parts path_parts = {0}; + daos_handle_t coh; assert_success(ddb_test_setup_vos(state)); /* test setup creates the pool, but doesn't open it ... leave it open for these tests */ tctx = *state; - assert_success(parse_vos_file_parts(tctx->dvt_pmem_file, NULL, &path_parts)); - assert_success(dv_pool_open(tctx->dvt_pmem_file, &path_parts, &tctx->dvt_poh, 0, true)); - + ctx.dc_write_mode = true; + assert_success(dv_pool_open(tctx->dvt_pmem_file, NULL, &ctx, 0)); + tctx->dvt_poh = ctx.dc_poh; g_ctx.dc_poh = tctx->dvt_poh; + assert_success(vos_cont_open(ctx.dc_poh, g_uuids[0], &coh)); + + dvt_vos_insert_2_records_with_dtx(coh); + vos_cont_close(coh); + return 0; } diff --git a/src/utils/ddb/tests/ddb_parse_tests.c b/src/utils/ddb/tests/ddb_parse_tests.c index 76560a6826b..9f14dad8f03 100644 --- a/src/utils/ddb/tests/ddb_parse_tests.c +++ b/src/utils/ddb/tests/ddb_parse_tests.c @@ -20,6 +20,7 @@ */ #define MOCKED_POOL_UUID_STR "12345678-1234-1234-1234-123456789012" +#define MOCKED_VOS_PATH_STR "/" MOCKED_POOL_UUID_STR "/vos-0" static int fake_print(const char *fmt, ...) @@ -74,16 +75,39 @@ parse_vos_file_parts_test_errors(void **state) rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-01", NULL, &parts); assert_rc_equal(rc, -DER_INVAL); - /* Test invalid vos paths with too long db path */ + /* Test invalid vos paths with too long vos path */ + D_ALLOC_ARRAY_CHECK(buf, VOS_PATH_SIZE + 1); + memset(buf, 'a', VOS_PATH_SIZE + 1); + buf[0] = '/'; + memcpy(&buf[VOS_PATH_SIZE + 1 - sizeof(MOCKED_VOS_PATH_STR)], MOCKED_VOS_PATH_STR, + sizeof(MOCKED_VOS_PATH_STR)); + rc = parse_vos_file_parts(buf, NULL, &parts); + D_FREE(buf); + assert_rc_equal(rc, -DER_EXCEEDS_PATH_LEN); + + /* Test vos_path whose directory component exceeds DB_PATH_SIZE. + * Note: this does not directly test the DB_PATH_SIZE check since + * DB_PATH_SIZE == VOS_PATH_SIZE means the VOS_PATH_SIZE check fires first. + * This test remains relevant in case the two constants diverge in the future. */ D_ALLOC_ARRAY_CHECK(buf, DB_PATH_SIZE + 64); memset(buf, 'a', DB_PATH_SIZE + 64); buf[0] = '/'; - memcpy(&buf[DB_PATH_SIZE], "/" MOCKED_POOL_UUID_STR "/vos-0", - sizeof("/" MOCKED_POOL_UUID_STR "/vos-0")); + memcpy(&buf[DB_PATH_SIZE], MOCKED_VOS_PATH_STR, sizeof(MOCKED_VOS_PATH_STR)); rc = parse_vos_file_parts(buf, NULL, &parts); D_FREE(buf); assert_rc_equal(rc, -DER_EXCEEDS_PATH_LEN); + /* Test invalid db path in MD-on-SSD mode: in MD-on-SSD mode db_path is explicitly + * provided by the user, allowing the DB_PATH_SIZE check to be tested independently + * of the VOS_PATH_SIZE check. */ + D_ALLOC_ARRAY_CHECK(buf, DB_PATH_SIZE + 1); + memset(buf, 'a', DB_PATH_SIZE); + buf[0] = '/'; + buf[DB_PATH_SIZE] = '\0'; + rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", buf, &parts); + D_FREE(buf); + assert_rc_equal(rc, -DER_EXCEEDS_PATH_LEN); + /* Test invalid vos paths with too long vos file name */ rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-999999999999", NULL, &parts); @@ -103,15 +127,6 @@ parse_vos_file_parts_test_errors(void **state) rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-99999999999", NULL, &parts); assert_rc_equal(rc, -DER_OVERFLOW); - - /* Test invalid vos paths with too long db path - MD-on-SSD */ - D_ALLOC_ARRAY_CHECK(buf, DB_PATH_SIZE + 1); - memset(buf, 'a', DB_PATH_SIZE); - buf[0] = '/'; - buf[DB_PATH_SIZE] = '\0'; - rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", buf, &parts); - D_FREE(buf); - assert_rc_equal(rc, -DER_EXCEEDS_PATH_LEN); } static void @@ -127,6 +142,7 @@ parse_vos_file_parts_test_success(void **state) /* Test with root path */ rc = parse_vos_file_parts("/" MOCKED_POOL_UUID_STR "/vos-0", NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal("/" MOCKED_POOL_UUID_STR "/vos-0", parts.vf_vos_file_path); assert_string_equal("/", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-0", parts.vf_vos_file_name); @@ -135,6 +151,7 @@ parse_vos_file_parts_test_success(void **state) /* Test with absolute path */ rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", parts.vf_vos_file_path); assert_string_equal("/mnt/daos", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-0", parts.vf_vos_file_name); @@ -144,6 +161,8 @@ parse_vos_file_parts_test_success(void **state) rc = parse_vos_file_parts("//////mnt////daos/////" MOCKED_POOL_UUID_STR "/////vos-0", NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal("//////mnt////daos/////" MOCKED_POOL_UUID_STR "/////vos-0", + parts.vf_vos_file_path); assert_string_equal("//////mnt////daos", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-0", parts.vf_vos_file_name); @@ -153,6 +172,7 @@ parse_vos_file_parts_test_success(void **state) memset(&parts, 0, sizeof(parts)); rc = parse_vos_file_parts("mnt/daos/" MOCKED_POOL_UUID_STR "/vos-42", NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal("mnt/daos/" MOCKED_POOL_UUID_STR "/vos-42", parts.vf_vos_file_path); assert_string_equal("mnt/daos", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-42", parts.vf_vos_file_name); @@ -161,6 +181,7 @@ parse_vos_file_parts_test_success(void **state) /* Test with relative path */ rc = parse_vos_file_parts("./" MOCKED_POOL_UUID_STR "/rdb-pool", NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal("./" MOCKED_POOL_UUID_STR "/rdb-pool", parts.vf_vos_file_path); assert_string_equal(".", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("rdb-pool", parts.vf_vos_file_name); @@ -170,6 +191,7 @@ parse_vos_file_parts_test_success(void **state) memset(&parts, 1, sizeof(parts)); rc = parse_vos_file_parts(MOCKED_POOL_UUID_STR "/vos-909", NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal(MOCKED_POOL_UUID_STR "/vos-909", parts.vf_vos_file_path); assert_string_equal(".", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-909", parts.vf_vos_file_name); @@ -179,6 +201,7 @@ parse_vos_file_parts_test_success(void **state) rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", "/my/db/path", &parts); assert_rc_equal(rc, DER_SUCCESS); + assert_string_equal("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-0", parts.vf_vos_file_path); assert_string_equal("/my/db/path", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-0", parts.vf_vos_file_name); diff --git a/src/utils/ddb/tests/ddb_test_driver.c b/src/utils/ddb/tests/ddb_test_driver.c index 3cfcfc135b6..bbaa3ae8c5f 100644 --- a/src/utils/ddb/tests/ddb_test_driver.c +++ b/src/utils/ddb/tests/ddb_test_driver.c @@ -304,8 +304,8 @@ ddb_test_setup_vos(void **state) int ddb_teardown_vos(void **state) { - struct dt_vos_pool_ctx *tctx = *state; - struct vos_file_parts path_parts = {0}; + struct ddb_ctx ctx = {0}; + struct dt_vos_pool_ctx *tctx = *state; int rc = 0; if (tctx == NULL) { @@ -314,8 +314,7 @@ ddb_teardown_vos(void **state) } if (tctx->dvt_special_pool_destroy) { - assert_success(parse_vos_file_parts(tctx->dvt_pmem_file, NULL, &path_parts)); - rc = dv_pool_destroy(tctx->dvt_pmem_file, &path_parts); + rc = dv_pool_destroy(tctx->dvt_pmem_file, NULL, &ctx); } else { vos_self_init("/mnt/daos", false, 0); assert_success(vos_pool_destroy(tctx->dvt_pmem_file, tctx->dvt_pool_uuid)); diff --git a/src/utils/ddb/tests/ddb_vos_tests.c b/src/utils/ddb/tests/ddb_vos_tests.c index 6297f55395f..f8e931453ec 100644 --- a/src/utils/ddb/tests/ddb_vos_tests.c +++ b/src/utils/ddb/tests/ddb_vos_tests.c @@ -182,18 +182,15 @@ __assert_ddb_iterate(daos_handle_t poh, uuid_t *cont_uuid, daos_unit_oid_t *oid, static void open_pool_test(void **state) { - daos_handle_t poh; - struct dt_vos_pool_ctx *tctx = *state; - struct vos_file_parts path_parts = {0}; - - assert_success(parse_vos_file_parts(tctx->dvt_pmem_file, NULL, &path_parts)); + struct ddb_ctx ctx = {0}; + struct dt_vos_pool_ctx *tctx = *state; - assert_success(dv_pool_open(tctx->dvt_pmem_file, &path_parts, &poh, 0, false)); - assert_success(dv_pool_close(poh)); + assert_success(dv_pool_open(tctx->dvt_pmem_file, NULL, &ctx, 0)); + assert_success(dv_pool_close(ctx.dc_poh)); /* should be able to open again after closing */ - assert_success(dv_pool_open(tctx->dvt_pmem_file, &path_parts, &poh, 0, false)); - assert_success(dv_pool_close(poh)); + assert_success(dv_pool_open(tctx->dvt_pmem_file, NULL, &ctx, 0)); + assert_success(dv_pool_close(ctx.dc_poh)); } static void @@ -1087,14 +1084,14 @@ dv_suit_teardown(void **state) static int dv_test_setup(void **state) { + struct ddb_ctx ctx = {0}; struct dt_vos_pool_ctx *tctx = *state; - struct vos_file_parts path_parts = {0}; - - assert_success(parse_vos_file_parts(tctx->dvt_pmem_file, NULL, &path_parts)); + ctx.dc_write_mode = true; active_entry_handler_called = 0; committed_entry_handler_called = 0; - assert_success(dv_pool_open(tctx->dvt_pmem_file, &path_parts, &tctx->dvt_poh, 0, true)); + assert_success(dv_pool_open(tctx->dvt_pmem_file, NULL, &ctx, 0)); + tctx->dvt_poh = ctx.dc_poh; return 0; } @@ -1110,24 +1107,22 @@ dv_test_teardown(void **state) static void pool_flags_tests(void **state) { - daos_handle_t poh; + struct ddb_ctx ctx = {0}; struct dt_vos_pool_ctx *tctx = *state; - struct vos_file_parts path_parts = {0}; uint64_t compat_flags; uint64_t incompat_flags; - assert_success(parse_vos_file_parts(tctx->dvt_pmem_file, NULL, &path_parts)); - assert_success( - dv_pool_open(tctx->dvt_pmem_file, &path_parts, &poh, VOS_POF_FOR_FEATURE_FLAG, true)); - assert_success(dv_pool_get_flags(poh, &compat_flags, &incompat_flags)); + ctx.dc_write_mode = true; + assert_success(dv_pool_open(tctx->dvt_pmem_file, NULL, &ctx, VOS_POF_FOR_FEATURE_FLAG)); + assert_success(dv_pool_get_flags(ctx.dc_poh, &compat_flags, &incompat_flags)); assert(compat_flags == 0); assert(incompat_flags == 0); - assert_success( - dv_pool_update_flags(poh, VOS_POOL_COMPAT_FLAG_SUPP, VOS_POOL_INCOMPAT_FLAG_SUPP)); - assert_success(dv_pool_get_flags(poh, &compat_flags, &incompat_flags)); + assert_success(dv_pool_update_flags(ctx.dc_poh, VOS_POOL_COMPAT_FLAG_SUPP, + VOS_POOL_INCOMPAT_FLAG_SUPP)); + assert_success(dv_pool_get_flags(ctx.dc_poh, &compat_flags, &incompat_flags)); assert(compat_flags == VOS_POOL_COMPAT_FLAG_SUPP); assert(incompat_flags == VOS_POOL_INCOMPAT_FLAG_SUPP); - assert_success(dv_pool_close(poh)); + assert_success(dv_pool_close(ctx.dc_poh)); } #define SHA256_DIGEST_LEN 64 @@ -1176,16 +1171,17 @@ static void helper_stat_open_modify_close_stat(struct dt_vos_pool_ctx *tctx, struct file_state fs[2], bool write_mode) { - const char *path = tctx->dvt_pmem_file; - struct vos_file_parts path_parts = {0}; + struct ddb_ctx ctx = {0}; + const char *path = tctx->dvt_pmem_file; assert_int_equal(stat(path, &fs[FILE_STATE_PRE].stat), 0); sha256sum(path, fs[FILE_STATE_PRE].digest); - assert_success(parse_vos_file_parts(path, NULL, &path_parts)); - assert_success(dv_pool_open(path, &path_parts, &tctx->dvt_poh, 0, write_mode)); + ctx.dc_write_mode = write_mode; + assert_success(dv_pool_open(path, NULL, &ctx, 0)); + tctx->dvt_poh = ctx.dc_poh; update_value_to_modify_tests((void **)&tctx); - assert_success(dv_pool_close(tctx->dvt_poh)); + assert_success(dv_pool_close(ctx.dc_poh)); assert_int_equal(stat(path, &fs[FILE_STATE_POST].stat), 0); sha256sum(path, fs[FILE_STATE_POST].digest); From b6f81253e1925e7a789b7e047c750808f96ec650 Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 10:08:21 +0000 Subject: [PATCH 4/9] DAOS-18304 ddb: introduce DdbAPI interface and convert DdbContext to methods Add the DdbAPI interface that lists every ddb sub-command as a method. It decouples the command layer from the concrete CGo implementation so tests can inject a stub without a live VOS environment. Convert all ddbXxx() free functions to (ctx *DdbContext) Xxx() methods, matching the DdbAPI interface. InitDdb() becomes (ctx *DdbContext) Init() so the caller (or a stub) controls object allocation. Open(), Feature() and RmPool() now receive db_path as an explicit argument instead of reading it from ctx.ctx.dc_db_path (which was removed from the C struct in the previous commit). Signed-off-by: Cedric Koch-Hofer --- src/control/cmd/ddb/commands_wrapper.go | 158 ++++++++++++++---------- 1 file changed, 91 insertions(+), 67 deletions(-) diff --git a/src/control/cmd/ddb/commands_wrapper.go b/src/control/cmd/ddb/commands_wrapper.go index d9751dbd8e5..957e3d55ab5 100644 --- a/src/control/cmd/ddb/commands_wrapper.go +++ b/src/control/cmd/ddb/commands_wrapper.go @@ -37,46 +37,67 @@ func freeString(s *C.char) { C.free(unsafe.Pointer(s)) } -func SetCString(out **C.char, s string) func() { - cstr := C.CString(s) - *out = cstr +type DdbAPI interface { + Init(log *logging.LeveledLogger) (func(), error) + PoolIsOpen() bool + Ls(path string, recursive bool, details bool) error + Open(path string, dbPath string, writeMode bool) error + Version() error + Close() error + SuperblockDump() error + ValueDump(path string, dst string) error + Rm(path string) error + ValueLoad(src string, dst string) error + IlogDump(path string) error + IlogCommit(path string) error + IlogClear(path string) error + DtxDump(path string, active bool, committed bool) error + DtxCmtClear(path string) error + SmdSync(nvmeConf string, dbPath string) error + VeaDump() error + VeaUpdate(offset string, blkCnt string) error + DtxActCommit(path string, dtxID string) error + DtxActAbort(path string, dtxID string) error + Feature(path, dbPath, enable, disable string, show bool) error + RmPool(path string, dbPath string) error + DtxActDiscardInvalid(path string, dtxID string) error + DevList(dbPath string) error + DevReplace(dbPath string, oldDevid string, newDevid string) error + DtxStat(path string, details bool) error + ProvMem(dbPath string, tmpfsMount string, tmpfsMountSize uint) error + DtxAggr(path string, cmtTime uint64, cmtDate string) error +} - return func() { - C.free(unsafe.Pointer(cstr)) - } +// DdbContext structure for wrapping the C code context structure +type DdbContext struct { + ctx C.struct_ddb_ctx + log *logging.LeveledLogger } -// InitDdb initializes the ddb context and returns a closure to finalize it. -func InitDdb(log *logging.LeveledLogger) (*DdbContext, func(), error) { +// Init initializes the ddb context and returns a closure to finalize it. +func (ctx *DdbContext) Init(log *logging.LeveledLogger) (func(), error) { // Must lock to OS thread because vos init/fini uses ABT init and finalize which must be called on the same thread runtime.LockOSThread() if err := daosError(C.ddb_init()); err != nil { runtime.UnlockOSThread() - return nil, nil, err + return nil, err } - ctx := &DdbContext{} C.ddb_ctx_init(&ctx.ctx) // Initialize with ctx default values ctx.log = log - return ctx, func() { + return func() { C.ddb_fini() runtime.UnlockOSThread() }, nil } -// DdbContext structure for wrapping the C code context structure -type DdbContext struct { - ctx C.struct_ddb_ctx - log *logging.LeveledLogger -} - -func ddbPoolIsOpen(ctx *DdbContext) bool { +func (ctx *DdbContext) PoolIsOpen() bool { return bool(C.ddb_pool_is_open(&ctx.ctx)) } -func ddbLs(ctx *DdbContext, path string, recursive bool, details bool) error { +func (ctx *DdbContext) Ls(path string, recursive bool, details bool) error { /* Set up the options */ options := C.struct_ls_options{} options.path = C.CString(path) @@ -87,33 +108,34 @@ func ddbLs(ctx *DdbContext, path string, recursive bool, details bool) error { return daosError(C.ddb_run_ls(&ctx.ctx, &options)) } -func ddbOpen(ctx *DdbContext, path string, write_mode bool) error { +func (ctx *DdbContext) Open(path string, dbPath string, writeMode bool) error { /* Set up the options */ options := C.struct_open_options{} options.path = C.CString(path) defer freeString(options.path) - options.db_path = ctx.ctx.dc_db_path - options.write_mode = C.bool(write_mode) + options.db_path = C.CString(dbPath) + defer freeString(options.db_path) + options.write_mode = C.bool(writeMode) /* Run the c code command */ return daosError(C.ddb_run_open(&ctx.ctx, &options)) } -func ddbVersion(ctx *DdbContext) error { +func (ctx *DdbContext) Version() error { /* Run the c code command */ return daosError(C.ddb_run_version(&ctx.ctx)) } -func ddbClose(ctx *DdbContext) error { +func (ctx *DdbContext) Close() error { /* Run the c code command */ return daosError(C.ddb_run_close(&ctx.ctx)) } -func ddbSuperblockDump(ctx *DdbContext) error { +func (ctx *DdbContext) SuperblockDump() error { /* Run the c code command */ return daosError(C.ddb_run_superblock_dump(&ctx.ctx)) } -func ddbValueDump(ctx *DdbContext, path string, dst string) error { +func (ctx *DdbContext) ValueDump(path string, dst string) error { /* Set up the options */ options := C.struct_value_dump_options{} options.path = C.CString(path) @@ -124,7 +146,7 @@ func ddbValueDump(ctx *DdbContext, path string, dst string) error { return daosError(C.ddb_run_value_dump(&ctx.ctx, &options)) } -func ddbRm(ctx *DdbContext, path string) error { +func (ctx *DdbContext) Rm(path string) error { /* Set up the options */ options := C.struct_rm_options{} options.path = C.CString(path) @@ -133,7 +155,7 @@ func ddbRm(ctx *DdbContext, path string) error { return daosError(C.ddb_run_rm(&ctx.ctx, &options)) } -func ddbValueLoad(ctx *DdbContext, src string, dst string) error { +func (ctx *DdbContext) ValueLoad(src string, dst string) error { /* Set up the options */ options := C.struct_value_load_options{} options.src = C.CString(src) @@ -144,7 +166,7 @@ func ddbValueLoad(ctx *DdbContext, src string, dst string) error { return daosError(C.ddb_run_value_load(&ctx.ctx, &options)) } -func ddbIlogDump(ctx *DdbContext, path string) error { +func (ctx *DdbContext) IlogDump(path string) error { /* Set up the options */ options := C.struct_ilog_dump_options{} options.path = C.CString(path) @@ -153,7 +175,7 @@ func ddbIlogDump(ctx *DdbContext, path string) error { return daosError(C.ddb_run_ilog_dump(&ctx.ctx, &options)) } -func ddbIlogCommit(ctx *DdbContext, path string) error { +func (ctx *DdbContext) IlogCommit(path string) error { /* Set up the options */ options := C.struct_ilog_commit_options{} options.path = C.CString(path) @@ -162,7 +184,7 @@ func ddbIlogCommit(ctx *DdbContext, path string) error { return daosError(C.ddb_run_ilog_commit(&ctx.ctx, &options)) } -func ddbIlogClear(ctx *DdbContext, path string) error { +func (ctx *DdbContext) IlogClear(path string) error { /* Set up the options */ options := C.struct_ilog_clear_options{} options.path = C.CString(path) @@ -171,7 +193,7 @@ func ddbIlogClear(ctx *DdbContext, path string) error { return daosError(C.ddb_run_ilog_clear(&ctx.ctx, &options)) } -func ddbDtxDump(ctx *DdbContext, path string, active bool, committed bool) error { +func (ctx *DdbContext) DtxDump(path string, active bool, committed bool) error { /* Set up the options */ options := C.struct_dtx_dump_options{} options.path = C.CString(path) @@ -182,7 +204,7 @@ func ddbDtxDump(ctx *DdbContext, path string, active bool, committed bool) error return daosError(C.ddb_run_dtx_dump(&ctx.ctx, &options)) } -func ddbDtxCmtClear(ctx *DdbContext, path string) error { +func (ctx *DdbContext) DtxCmtClear(path string) error { /* Set up the options */ options := C.struct_dtx_cmt_clear_options{} options.path = C.CString(path) @@ -191,61 +213,62 @@ func ddbDtxCmtClear(ctx *DdbContext, path string) error { return daosError(C.ddb_run_dtx_cmt_clear(&ctx.ctx, &options)) } -func ddbSmdSync(ctx *DdbContext, nvme_conf string, db_path string) error { +func (ctx *DdbContext) SmdSync(nvmeConf string, dbPath string) error { /* Set up the options */ options := C.struct_smd_sync_options{} - options.nvme_conf = C.CString(nvme_conf) + options.nvme_conf = C.CString(nvmeConf) defer freeString(options.nvme_conf) - options.db_path = C.CString(db_path) + options.db_path = C.CString(dbPath) defer freeString(options.db_path) /* Run the c code command */ return daosError(C.ddb_run_smd_sync(&ctx.ctx, &options)) } -func ddbVeaDump(ctx *DdbContext) error { +func (ctx *DdbContext) VeaDump() error { /* Run the c code command */ return daosError(C.ddb_run_vea_dump(&ctx.ctx)) } -func ddbVeaUpdate(ctx *DdbContext, offset string, blk_cnt string) error { +func (ctx *DdbContext) VeaUpdate(offset string, blkCnt string) error { /* Set up the options */ options := C.struct_vea_update_options{} options.offset = C.CString(offset) defer freeString(options.offset) - options.blk_cnt = C.CString(blk_cnt) + options.blk_cnt = C.CString(blkCnt) defer freeString(options.blk_cnt) /* Run the c code command */ return daosError(C.ddb_run_vea_update(&ctx.ctx, &options)) } -func ddbDtxActCommit(ctx *DdbContext, path string, dtx_id string) error { +func (ctx *DdbContext) DtxActCommit(path string, dtxID string) error { /* Set up the options */ options := C.struct_dtx_act_options{} options.path = C.CString(path) defer freeString(options.path) - options.dtx_id = C.CString(dtx_id) + options.dtx_id = C.CString(dtxID) defer freeString(options.dtx_id) /* Run the c code command */ return daosError(C.ddb_run_dtx_act_commit(&ctx.ctx, &options)) } -func ddbDtxActAbort(ctx *DdbContext, path string, dtx_id string) error { +func (ctx *DdbContext) DtxActAbort(path string, dtxID string) error { /* Set up the options */ options := C.struct_dtx_act_options{} options.path = C.CString(path) defer freeString(options.path) - options.dtx_id = C.CString(dtx_id) + options.dtx_id = C.CString(dtxID) defer freeString(options.dtx_id) /* Run the c code command */ return daosError(C.ddb_run_dtx_act_abort(&ctx.ctx, &options)) } -func ddbFeature(ctx *DdbContext, path, enable, disable string, show bool) error { +func (ctx *DdbContext) Feature(path, dbPath, enable, disable string, show bool) error { /* Set up the options */ options := C.struct_feature_options{} options.path = C.CString(path) defer freeString(options.path) - options.db_path = ctx.ctx.dc_db_path + options.db_path = C.CString(dbPath) + defer freeString(options.db_path) if enable != "" { err := daosError(C.ddb_feature_string2flags(&ctx.ctx, C.CString(enable), &options.set_compat_flags, &options.set_incompat_flags)) @@ -265,50 +288,51 @@ func ddbFeature(ctx *DdbContext, path, enable, disable string, show bool) error return daosError(C.ddb_run_feature(&ctx.ctx, &options)) } -func ddbRmPool(ctx *DdbContext, path string) error { +func (ctx *DdbContext) RmPool(path string, dbPath string) error { /* Set up the options */ options := C.struct_rm_pool_options{} options.path = C.CString(path) defer freeString(options.path) - options.db_path = ctx.ctx.dc_db_path + options.db_path = C.CString(dbPath) + defer freeString(options.db_path) /* Run the c code command */ return daosError(C.ddb_run_rm_pool(&ctx.ctx, &options)) } -func ddbDtxActDiscardInvalid(ctx *DdbContext, path string, dtx_id string) error { +func (ctx *DdbContext) DtxActDiscardInvalid(path string, dtxID string) error { /* Set up the options */ options := C.struct_dtx_act_options{} options.path = C.CString(path) defer freeString(options.path) - options.dtx_id = C.CString(dtx_id) + options.dtx_id = C.CString(dtxID) defer freeString(options.dtx_id) /* Run the c code command */ return daosError(C.ddb_run_dtx_act_discard_invalid(&ctx.ctx, &options)) } -func ddbDevList(ctx *DdbContext, db_path string) error { +func (ctx *DdbContext) DevList(dbPath string) error { /* Set up the options */ options := C.struct_dev_list_options{} - options.db_path = C.CString(db_path) + options.db_path = C.CString(dbPath) defer freeString(options.db_path) /* Run the c code command */ return daosError(C.ddb_run_dev_list(&ctx.ctx, &options)) } -func ddbDevReplace(ctx *DdbContext, db_path string, old_devid string, new_devid string) error { +func (ctx *DdbContext) DevReplace(dbPath string, oldDevID string, newDevID string) error { /* Set up the options */ options := C.struct_dev_replace_options{} - options.db_path = C.CString(db_path) + options.db_path = C.CString(dbPath) defer freeString(options.db_path) - options.old_devid = C.CString(old_devid) + options.old_devid = C.CString(oldDevID) defer freeString(options.old_devid) - options.new_devid = C.CString(new_devid) + options.new_devid = C.CString(newDevID) defer freeString(options.new_devid) /* Run the c code command */ return daosError(C.ddb_run_dev_replace(&ctx.ctx, &options)) } -func ddbDtxStat(ctx *DdbContext, path string, details bool) error { +func (ctx *DdbContext) DtxStat(path string, details bool) error { /* Set up the options */ options := C.struct_dtx_stat_options{} options.path = C.CString(path) @@ -318,25 +342,25 @@ func ddbDtxStat(ctx *DdbContext, path string, details bool) error { return daosError(C.ddb_run_dtx_stat(&ctx.ctx, &options)) } -func ddbProvMem(ctx *DdbContext, db_path string, tmpfs_mount string, tmpfs_mount_size uint) error { +func (ctx *DdbContext) ProvMem(dbPath string, tmpfsMount string, tmpfsMountSize uint) error { /* Set up the options */ options := C.struct_prov_mem_options{} - options.db_path = C.CString(db_path) + options.db_path = C.CString(dbPath) defer freeString(options.db_path) - options.tmpfs_mount = C.CString(tmpfs_mount) + options.tmpfs_mount = C.CString(tmpfsMount) defer freeString(options.tmpfs_mount) - options.tmpfs_mount_size = C.uint(tmpfs_mount_size) + options.tmpfs_mount_size = C.uint(tmpfsMountSize) /* Run the c code command */ return daosError(C.ddb_run_prov_mem(&ctx.ctx, &options)) } -func ddbDtxAggr(ctx *DdbContext, path string, cmt_time uint64, cmt_date string) error { - if cmt_time != math.MaxUint64 && cmt_date != "" { +func (ctx *DdbContext) DtxAggr(path string, cmtTime uint64, cmtDate string) error { + if cmtTime != math.MaxUint64 && cmtDate != "" { ctx.log.Error("'--cmt_time' and '--cmt_date' options are mutually exclusive") return daosError(-C.DER_INVAL) } - if cmt_time == math.MaxUint64 && cmt_date == "" { + if cmtTime == math.MaxUint64 && cmtDate == "" { ctx.log.Error("'--cmt_time' or '--cmt_date' option has to be defined") return daosError(-C.DER_INVAL) } @@ -345,13 +369,13 @@ func ddbDtxAggr(ctx *DdbContext, path string, cmt_time uint64, cmt_date string) options := C.struct_dtx_aggr_options{} options.path = C.CString(path) defer freeString(options.path) - if cmt_time != math.MaxUint64 { + if cmtTime != math.MaxUint64 { options.format = C.DDB_DTX_AGGR_CMT_TIME - options.cmt_time = C.uint64_t(cmt_time) + options.cmt_time = C.uint64_t(cmtTime) } - if cmt_date != "" { + if cmtDate != "" { options.format = C.DDB_DTX_AGGR_CMT_DATE - options.cmt_date = C.CString(cmt_date) + options.cmt_date = C.CString(cmtDate) defer freeString(options.cmt_date) } /* Run the c code command */ From 5ca13e018e3fbbe1686b68e77c7c5e52761cdc86 Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 10:08:39 +0000 Subject: [PATCH 5/9] DAOS-18304 ddb: propagate DdbAPI through command and CLI layers ddb_commands.go: addAppCommands() now takes a DdbAPI instead of a *DdbContext; every command Run closure calls api.Xxx() instead of the former ddbXxx(ctx, ...) free functions. The 'open' and 'feature' commands pass db_path explicitly; 'rm_pool' now requires --vos_path. main.go: - parseOpts() no longer calls os.Exit(); it returns (cliOptions, *flags.Parser, error) so the function is fully unit-testable. - A new run() function owns the DDB context lifetime: Init/defer-cleanup, auto-open logic, non-interactive command dispatch, interactive loop. - --version is handled in main() before run(), printing directly instead of synthesising a fake command. - --db_path without --vos_path returns an explicit error. - The HasPrefix loop for auto-open exclusions is replaced by a noAutoOpen map for exact command-name matching. - Pool close is consolidated into a closePoolIfOpen() helper used with defer. - Errors from runCmdStr() and runFileCmds() are now propagated. - unknownCmdError typed struct enables type-safe detection; exitWithError() prints the command list on unknown commands and fixes the duplicated ERROR: prefix in fault resolution messages. - errHelpRequested sentinel avoids os.Exit() in the --help path. Signed-off-by: Cedric Koch-Hofer --- src/control/cmd/ddb/ddb_commands.go | 62 ++++---- src/control/cmd/ddb/main.go | 212 +++++++++++++++------------- 2 files changed, 142 insertions(+), 132 deletions(-) diff --git a/src/control/cmd/ddb/ddb_commands.go b/src/control/cmd/ddb/ddb_commands.go index d515499264c..28c743e5db4 100644 --- a/src/control/cmd/ddb/ddb_commands.go +++ b/src/control/cmd/ddb/ddb_commands.go @@ -14,7 +14,7 @@ import ( "github.com/desertbit/grumble" ) -func addAppCommands(app *grumble.App, ctx *DdbContext) { +func addAppCommands(app *grumble.App, api DdbAPI) { // Command: ls app.AddCommand(&grumble.Command{ Name: "ls", @@ -30,7 +30,7 @@ func addAppCommands(app *grumble.App, ctx *DdbContext) { a.String("path", "Optional, list contents of the provided path", grumble.Default("")) }, Run: func(c *grumble.Context) error { - return ddbLs(ctx, c.Args.String("path"), c.Flags.Bool("recursive"), c.Flags.Bool("details")) + return api.Ls(c.Args.String("path"), c.Flags.Bool("recursive"), c.Flags.Bool("details")) }, Completer: nil, }) @@ -51,11 +51,7 @@ pool shard. Part of the path is used to determine what the pool uuid is.`, a.String("path", "Path to the vos file to open.") }, Run: func(c *grumble.Context) error { - if c.Flags.String("db_path") != "" { - cleanup := SetCString(&ctx.ctx.dc_db_path, c.Flags.String("db_path")) - defer cleanup() - } - return ddbOpen(ctx, c.Args.String("path"), c.Flags.Bool("write_mode")) + return api.Open(c.Args.String("path"), c.Flags.String("db_path"), c.Flags.Bool("write_mode")) }, Completer: openCompleter, }) @@ -67,7 +63,7 @@ pool shard. Part of the path is used to determine what the pool uuid is.`, LongHelp: "", HelpGroup: "", Run: func(c *grumble.Context) error { - return ddbVersion(ctx) + return api.Version() }, Completer: nil, }) @@ -79,7 +75,7 @@ pool shard. Part of the path is used to determine what the pool uuid is.`, LongHelp: "", HelpGroup: "vos", Run: func(c *grumble.Context) error { - return ddbClose(ctx) + return api.Close() }, Completer: nil, }) @@ -91,7 +87,7 @@ pool shard. Part of the path is used to determine what the pool uuid is.`, LongHelp: "", HelpGroup: "vos", Run: func(c *grumble.Context) error { - return ddbSuperblockDump(ctx) + return api.SuperblockDump() }, Completer: nil, }) @@ -110,7 +106,7 @@ the file, else it will be printed to the screen.`, a.String("dst", "File path to dump the value to.", grumble.Default("")) }, Run: func(c *grumble.Context) error { - return ddbValueDump(ctx, c.Args.String("path"), c.Args.String("dst")) + return api.ValueDump(c.Args.String("path"), c.Args.String("dst")) }, Completer: nil, }) @@ -126,7 +122,7 @@ and everything under it, to a single value.`, a.String("path", "VOS tree path to remove.") }, Run: func(c *grumble.Context) error { - return ddbRm(ctx, c.Args.String("path")) + return api.Rm(c.Args.String("path")) }, Completer: nil, }) @@ -147,7 +143,7 @@ the path must include the extent, otherwise, it must not.`, a.String("dst", "Destination vos tree path to a value.") }, Run: func(c *grumble.Context) error { - return ddbValueLoad(ctx, c.Args.String("src"), c.Args.String("dst")) + return api.ValueLoad(c.Args.String("src"), c.Args.String("dst")) }, Completer: nil, }) @@ -162,7 +158,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "VOS tree path to an object, dkey, or akey.") }, Run: func(c *grumble.Context) error { - return ddbIlogDump(ctx, c.Args.String("path")) + return api.IlogDump(c.Args.String("path")) }, Completer: nil, }) @@ -177,7 +173,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "VOS tree path to an object, dkey, or akey.") }, Run: func(c *grumble.Context) error { - return ddbIlogCommit(ctx, c.Args.String("path")) + return api.IlogCommit(c.Args.String("path")) }, Completer: nil, }) @@ -192,7 +188,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "VOS tree path to an object, dkey, or akey.") }, Run: func(c *grumble.Context) error { - return ddbIlogClear(ctx, c.Args.String("path")) + return api.IlogClear(c.Args.String("path")) }, Completer: nil, }) @@ -211,7 +207,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "VOS tree path to a container.") }, Run: func(c *grumble.Context) error { - return ddbDtxDump(ctx, c.Args.String("path"), c.Flags.Bool("active"), c.Flags.Bool("committed")) + return api.DtxDump(c.Args.String("path"), c.Flags.Bool("active"), c.Flags.Bool("committed")) }, Completer: nil, }) @@ -226,7 +222,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "VOS tree path to a container.") }, Run: func(c *grumble.Context) error { - return ddbDtxCmtClear(ctx, c.Args.String("path")) + return api.DtxCmtClear(c.Args.String("path")) }, Completer: nil, }) @@ -242,7 +238,7 @@ the path must include the extent, otherwise, it must not.`, a.String("db_path", "Path to the vos db. (default /mnt/daos)", grumble.Default("")) }, Run: func(c *grumble.Context) error { - return ddbSmdSync(ctx, c.Args.String("nvme_conf"), c.Args.String("db_path")) + return api.SmdSync(c.Args.String("nvme_conf"), c.Args.String("db_path")) }, Completer: nil, }) @@ -254,7 +250,7 @@ the path must include the extent, otherwise, it must not.`, LongHelp: "", HelpGroup: "vos", Run: func(c *grumble.Context) error { - return ddbVeaDump(ctx) + return api.VeaDump() }, Completer: nil, }) @@ -270,7 +266,7 @@ the path must include the extent, otherwise, it must not.`, a.String("blk_cnt", "Total blocks of the region to mark free.") }, Run: func(c *grumble.Context) error { - return ddbVeaUpdate(ctx, c.Args.String("offset"), c.Args.String("blk_cnt")) + return api.VeaUpdate(c.Args.String("offset"), c.Args.String("blk_cnt")) }, Completer: nil, }) @@ -286,7 +282,7 @@ the path must include the extent, otherwise, it must not.`, a.String("dtx_id", "DTX id of the entry to commit. ") }, Run: func(c *grumble.Context) error { - return ddbDtxActCommit(ctx, c.Args.String("path"), c.Args.String("dtx_id")) + return api.DtxActCommit(c.Args.String("path"), c.Args.String("dtx_id")) }, Completer: nil, }) @@ -302,7 +298,7 @@ the path must include the extent, otherwise, it must not.`, a.String("dtx_id", "DTX id of the entry to abort. ") }, Run: func(c *grumble.Context) error { - return ddbDtxActAbort(ctx, c.Args.String("path"), c.Args.String("dtx_id")) + return api.DtxActAbort(c.Args.String("path"), c.Args.String("dtx_id")) }, Completer: nil, }) @@ -323,7 +319,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "Optional, Path to the vos file", grumble.Default("")) }, Run: func(c *grumble.Context) error { - return ddbFeature(ctx, c.Args.String("path"), c.Flags.String("enable"), c.Flags.String("disable"), c.Flags.Bool("show")) + return api.Feature(c.Args.String("path"), c.Flags.String("db_path"), c.Flags.String("enable"), c.Flags.String("disable"), c.Flags.Bool("show")) }, Completer: featureCompleter, }) @@ -335,13 +331,13 @@ the path must include the extent, otherwise, it must not.`, LongHelp: "", HelpGroup: "vos", Flags: func(f *grumble.Flags) { - f.String("p", "db_path", "", "Path to the sys db.") + f.String("p", "db_path", "", "Path to the sys db") }, Args: func(a *grumble.Args) { - a.String("path", "Optional, Path to the vos file", grumble.Default("")) + a.String("path", "Path to the vos file") }, Run: func(c *grumble.Context) error { - return ddbRmPool(ctx, c.Args.String("path")) + return api.RmPool(c.Args.String("path"), c.Flags.String("db_path")) }, Completer: rmPoolCompleter, }) @@ -357,7 +353,7 @@ the path must include the extent, otherwise, it must not.`, a.String("dtx_id", "DTX id of the entry to validate or 'all' to validate all active DTX entries.") }, Run: func(c *grumble.Context) error { - return ddbDtxActDiscardInvalid(ctx, c.Args.String("path"), c.Args.String("dtx_id")) + return api.DtxActDiscardInvalid(c.Args.String("path"), c.Args.String("dtx_id")) }, Completer: nil, }) @@ -372,7 +368,7 @@ the path must include the extent, otherwise, it must not.`, a.String("db_path", "Path to the vos db.") }, Run: func(c *grumble.Context) error { - return ddbDevList(ctx, c.Args.String("db_path")) + return api.DevList(c.Args.String("db_path")) }, Completer: nil, }) @@ -389,7 +385,7 @@ the path must include the extent, otherwise, it must not.`, a.String("new_dev", "New device UUID.") }, Run: func(c *grumble.Context) error { - return ddbDevReplace(ctx, c.Args.String("db_path"), c.Args.String("old_dev"), c.Args.String("new_dev")) + return api.DevReplace(c.Args.String("db_path"), c.Args.String("old_dev"), c.Args.String("new_dev")) }, Completer: nil, }) @@ -407,7 +403,7 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "Optional, VOS tree path of a container to query.", grumble.Default("")) }, Run: func(c *grumble.Context) error { - return ddbDtxStat(ctx, c.Args.String("path"), c.Flags.Bool("details")) + return api.DtxStat(c.Args.String("path"), c.Flags.Bool("details")) }, Completer: nil, }) @@ -426,7 +422,7 @@ the path must include the extent, otherwise, it must not.`, a.String("tmpfs_mount", "Path to the tmpfs mountpoint.") }, Run: func(c *grumble.Context) error { - return ddbProvMem(ctx, c.Args.String("db_path"), c.Args.String("tmpfs_mount"), c.Flags.Uint("tmpfs_size")) + return api.ProvMem(c.Args.String("db_path"), c.Args.String("tmpfs_mount"), c.Flags.Uint("tmpfs_size")) }, Completer: nil, }) @@ -445,7 +441,7 @@ the path must include the extent, otherwise, it must not.`, f.String("d", "cmt_date", "", "Max aggregation committed date (format '1970-01-01 00:00:00')") }, Run: func(c *grumble.Context) error { - return ddbDtxAggr(ctx, c.Args.String("path"), c.Flags.Uint64("cmt_time"), c.Flags.String("cmt_date")) + return api.DtxAggr(c.Args.String("path"), c.Flags.Uint64("cmt_time"), c.Flags.String("cmt_date")) }, Completer: nil, }) diff --git a/src/control/cmd/ddb/main.go b/src/control/cmd/ddb/main.go index 9d20ce3b3d1..d21dcb7f78e 100644 --- a/src/control/cmd/ddb/main.go +++ b/src/control/cmd/ddb/main.go @@ -12,7 +12,6 @@ import ( "fmt" "io" "os" - "path" "path/filepath" "runtime/debug" "strings" @@ -30,11 +29,27 @@ import ( "github.com/daos-stack/daos/src/control/server/engine" ) +var errHelpRequested = errors.New("help requested") + +type unknownCmdError struct { + cmd string +} + +func (e *unknownCmdError) Error() string { + return fmt.Sprintf("Error running command '%s' unknown command, try 'help'", e.cmd) +} + func exitWithError(err error) { - cmdName := path.Base(os.Args[0]) - fmt.Fprintf(os.Stderr, "ERROR: %s: %v\n", cmdName, err) + cmdName := filepath.Base(os.Args[0]) + msg := fmt.Sprintf("ERROR: %s: %v", cmdName, err) if fault.HasResolution(err) { - fmt.Fprintf(os.Stderr, "ERROR: %s: %s", cmdName, fault.ShowResolutionFor(err)) + msg = fmt.Sprintf("%s (%s)", msg, fault.ShowResolutionFor(err)) + } + fmt.Fprintln(os.Stderr, msg) + + if _, ok := err.(*unknownCmdError); ok { + app := createGrumbleApp(nil) + printCommands(os.Stderr, app) } os.Exit(1) } @@ -54,12 +69,12 @@ type cliOptions struct { } const helpCommandsHeader = ` -Available commands: +Available Commands: ` const helpTreePath = ` -Path +Vos Tree Paths: Many of the commands take a VOS tree path. The format for this path is [cont]/[obj]/[dkey]/[akey]/[extent]. To make it easier to navigate the tree, indexes can be used @@ -87,8 +102,13 @@ MODE section of the manpage for details. ` const grumbleUnknownCmdErr = "unknown command, try 'help'" +const runCmdArgsErr = "Cannot use both command file and a command string" +const vosPathMissErr = "Cannot use sys db path without a vos path" +const loggerInitErr = "Logging facilities cannot be initialized" +const ctxInitErr = "DDB Context cannot be initialized" +const vosPathOpenErr = "Error opening VOS path '%s'" -func runFileCmds(log logging.Logger, app *grumble.App, fileName string) error { +func runFileCmds(app *grumble.App, log logging.Logger, fileName string) error { file, err := os.Open(fileName) if err != nil { return errors.Wrapf(err, "Error opening file %q", fileName) @@ -147,37 +167,25 @@ func printGeneralHelp(app *grumble.App, generalMsg string) { // Ask grumble to generate a help message for the requested command. // Caveat: There is no known easy way of forcing grumble to use log to print the generated message // so the output goes directly to stdout. -// Returns false in case the opts.Args.RunCmd is unknown. -func printCmdHelp(app *grumble.App, opts *cliOptions) bool { - err := runCmdStr(app, nil, string(opts.Args.RunCmd), "--help") - if err != nil { - if err.Error() == grumbleUnknownCmdErr { - fmt.Fprintf(os.Stderr, "ERROR: Unknown command '%s'", string(opts.Args.RunCmd)) - printCommands(os.Stderr, app) - } else { - fmt.Fprintf(os.Stderr, "ERROR: %s", err.Error()) - } - return false +func printCmdHelp(app *grumble.App, opts cliOptions) error { + if err := runCmdStr(app, nil, string(opts.Args.RunCmd), "--help"); err != nil { + return &unknownCmdError{cmd: opts.Args.RunCmd} } - return true + return errHelpRequested } // Prints either general or command-specific help message. // Returns a reasonable return code in case the caller chooses to terminate the process. -func printHelp(generalMsg string, opts *cliOptions) int { +func printHelp(opts cliOptions, generalMsg string) error { // ctx is not necessary since this instance of the app is not intended to run any of the commands app := createGrumbleApp(nil) if string(opts.Args.RunCmd) == "" { printGeneralHelp(app, generalMsg) - return 0 + return errHelpRequested } - if printCmdHelp(app, opts) { - return 0 - } else { - return 1 - } + return printCmdHelp(app, opts) } func setenvIfNotSet(key, value string) { @@ -216,7 +224,7 @@ func strToLogLevels(level string) (logging.LogLevel, engine.LogLevel, error) { } } -func newLogger(opts *cliOptions) (*logging.LeveledLogger, error) { +func newLogger(opts cliOptions) (*logging.LeveledLogger, error) { level := "ERR" if opts.Debug != "" { level = opts.Debug @@ -261,94 +269,83 @@ func newLogger(opts *cliOptions) (*logging.LeveledLogger, error) { return fileLog, nil } -func parseOpts(args []string, opts *cliOptions) error { - p := flags.NewParser(opts, flags.HelpFlag|flags.IgnoreUnknown) - p.Name = "ddb" - p.Usage = "[OPTIONS]" - p.ShortDescription = "daos debug tool" - p.LongDescription = ddbLongDescription +func closePoolIfOpen(api DdbAPI, log *logging.LeveledLogger) { + if !api.PoolIsOpen() { + return + } - // Set the traceback level such that a crash results in - // a coredump (when ulimit -c is set appropriately). - debug.SetTraceback("crash") + log.Info("Closing pool...\n") + if err := api.Close(); err != nil { + log.Errorf("Error closing pool: %s\n", err) + } +} - if _, err := p.ParseArgs(args); err != nil { +func parseOpts(args []string, api DdbAPI) (cliOptions, *flags.Parser, error) { + var opts cliOptions + parser := flags.NewParser(&opts, flags.HelpFlag|flags.IgnoreUnknown) + parser.Name = "ddb" + parser.Usage = "[OPTIONS]" + parser.ShortDescription = "daos debug tool" + parser.LongDescription = ddbLongDescription + + if _, err := parser.ParseArgs(args); err != nil { if fe, ok := errors.Cause(err).(*flags.Error); ok && fe.Type == flags.ErrHelp { - os.Exit(printHelp(fe.Error(), opts)) + return opts, nil, printHelp(opts, fe.Error()) } - return err - } - - if opts.Version { - opts.Args.RunCmd = "version" - opts.Args.RunCmdArgs = []string{} - opts.CmdFile = "" + return opts, nil, err } if opts.Args.RunCmd != "" && opts.CmdFile != "" { - return errors.New("Cannot use both command file and a command string") + return opts, nil, errors.New(runCmdArgsErr) } - log, err := newLogger(opts) - if err != nil { - return errors.Wrap(err, "Error configuring logging") + if opts.VosPath == "" && opts.SysdbPath != "" { + return opts, nil, errors.New(vosPathMissErr) } - log.Debug("Logging facilities initialized") - var ( - ctx *DdbContext - cleanup func() - ) - if ctx, cleanup, err = InitDdb(log); err != nil { - return errors.Wrap(err, "Error initializing the DDB Context") - } - defer cleanup() - app := createGrumbleApp(ctx) + return opts, parser, nil +} - if opts.SysdbPath != "" { - cleanup := SetCString(&ctx.ctx.dc_db_path, string(opts.SysdbPath)) - defer cleanup() +func run(api DdbAPI, log *logging.LeveledLogger, opts cliOptions, parser *flags.Parser) error { + cleanup, err := api.Init(log) + if err != nil { + return errors.Wrap(err, ctxInitErr) } + defer cleanup() + app := createGrumbleApp(api) if opts.VosPath != "" { - cleanup := SetCString(&ctx.ctx.dc_pool_path, string(opts.VosPath)) - defer cleanup() - - if !strings.HasPrefix(string(opts.Args.RunCmd), "feature") && - !strings.HasPrefix(string(opts.Args.RunCmd), "open") && - !strings.HasPrefix(string(opts.Args.RunCmd), "close") && - !strings.HasPrefix(string(opts.Args.RunCmd), "prov_mem") && - !strings.HasPrefix(string(opts.Args.RunCmd), "smd_sync") && - !strings.HasPrefix(string(opts.Args.RunCmd), "rm_pool") && - !strings.HasPrefix(string(opts.Args.RunCmd), "dev_list") && - !strings.HasPrefix(string(opts.Args.RunCmd), "dev_replace") { + // Commands that manage the pool open/close lifecycle themselves and must + // not have the pool pre-opened by the CLI layer. + noAutoOpen := map[string]bool{ + "feature": true, + "open": true, + "close": true, + "prov_mem": true, + "smd_sync": true, + "rm_pool": true, + "dev_list": true, + "dev_replace": true, + } + if !noAutoOpen[opts.Args.RunCmd] { log.Debugf("Connect to path: %s\n", opts.VosPath) - if err := ddbOpen(ctx, string(opts.VosPath), bool(opts.WriteMode)); err != nil { - return errors.Wrapf(err, "Error opening path: %s", opts.VosPath) + if err := api.Open(string(opts.VosPath), string(opts.SysdbPath), opts.WriteMode); err != nil { + return errors.Wrapf(err, vosPathOpenErr, opts.VosPath) } + defer closePoolIfOpen(api, log) } } if opts.Args.RunCmd != "" || opts.CmdFile != "" { // Non-interactive mode if opts.Args.RunCmd != "" { - err := runCmdStr(app, p, string(opts.Args.RunCmd), opts.Args.RunCmdArgs...) - if err != nil { - log.Errorf("Error running command %q %s\n", string(opts.Args.RunCmd), err) - } + err = runCmdStr(app, parser, string(opts.Args.RunCmd), opts.Args.RunCmdArgs...) } else { - err := runFileCmds(log, app, opts.CmdFile) - if err != nil { - log.Errorf("Error running command file: %s\n", err) - } + err = runFileCmds(app, log, opts.CmdFile) } - - if ddbPoolIsOpen(ctx) { - err := ddbClose(ctx) - if err != nil { - log.Error("Error closing pool\n") - } + if err != nil && err.Error() == grumbleUnknownCmdErr { + return &unknownCmdError{cmd: opts.Args.RunCmd} } return err } @@ -360,29 +357,46 @@ func parseOpts(args []string, opts *cliOptions) error { os.Args = []string{} result := app.Run() // make sure pool is closed - if ddbPoolIsOpen(ctx) { - err := ddbClose(ctx) - if err != nil { - log.Error("Error closing pool\n") - } - } + closePoolIfOpen(api, log) return result } func main() { - var opts cliOptions + // Set the traceback level such that a crash results in + // a coredump (when ulimit -c is set appropriately). + debug.SetTraceback("crash") // Must be called before any write to stdout. if err := logging.DisableCStdoutBuffering(); err != nil { exitWithError(err) } - if err := parseOpts(os.Args[1:], &opts); err != nil { + ctx := &DdbContext{} + opts, parser, err := parseOpts(os.Args[1:], ctx) + if errors.Is(err, errHelpRequested) { + return + } + if err != nil { + exitWithError(err) + } + + if opts.Version { + fmt.Printf("ddb version %s\n", build.DaosVersion) + return + } + + log, err := newLogger(opts) + if err != nil { + exitWithError(errors.Wrap(err, loggerInitErr)) + } + log.Debug("Logging facilities initialized") + + if err = run(ctx, log, opts, parser); err != nil { exitWithError(err) } } -func createGrumbleApp(ctx *DdbContext) *grumble.App { +func createGrumbleApp(api DdbAPI) *grumble.App { homedir, err := os.UserHomeDir() if err != nil { homedir = "/tmp" @@ -395,7 +409,7 @@ func createGrumbleApp(ctx *DdbContext) *grumble.App { Prompt: "ddb: ", }) - addAppCommands(app, ctx) + addAppCommands(app, api) // Add the quit command. grumble also includes a builtin exit command app.AddCommand(&grumble.Command{ From 7b3c6ac5b1750f5a9ef94c6a7531a3cfc31381d0 Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 13:12:58 +0000 Subject: [PATCH 6/9] DAOS-18304 ddb: use MOCKED_VOS_PATH_STR macro in Go unit tests Fix reviewers comments: - Use MOCKED_VOS_PATH_STR instead of the hardcoded "/" MOCKED_POOL_UUID_STR "/vos-0" string literal. Signed-off-by: Cedric Koch-Hofer --- src/utils/ddb/tests/ddb_parse_tests.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/utils/ddb/tests/ddb_parse_tests.c b/src/utils/ddb/tests/ddb_parse_tests.c index 9f14dad8f03..7db9e085a8e 100644 --- a/src/utils/ddb/tests/ddb_parse_tests.c +++ b/src/utils/ddb/tests/ddb_parse_tests.c @@ -140,9 +140,9 @@ parse_vos_file_parts_test_success(void **state) assert_rc_equal(rc, 0); /* Test with root path */ - rc = parse_vos_file_parts("/" MOCKED_POOL_UUID_STR "/vos-0", NULL, &parts); + rc = parse_vos_file_parts(MOCKED_VOS_PATH_STR, NULL, &parts); assert_rc_equal(rc, DER_SUCCESS); - assert_string_equal("/" MOCKED_POOL_UUID_STR "/vos-0", parts.vf_vos_file_path); + assert_string_equal(MOCKED_VOS_PATH_STR, parts.vf_vos_file_path); assert_string_equal("/", parts.vf_db_path); assert_uuid_equal(expected_uuid, parts.vf_pool_uuid); assert_string_equal("vos-0", parts.vf_vos_file_name); From b0f720750db9e3245cd01a77f40f127400feb2f7 Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Tue, 28 Apr 2026 15:25:14 +0000 Subject: [PATCH 7/9] DAOS-18304 ddb: replace noAutoOpen map with a string slice Fixed reviewers comments: - Use slice instead of dictionary Signed-off-by: Cedric Koch-Hofer --- src/control/cmd/ddb/main.go | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/control/cmd/ddb/main.go b/src/control/cmd/ddb/main.go index d21dcb7f78e..fdc846aca0f 100644 --- a/src/control/cmd/ddb/main.go +++ b/src/control/cmd/ddb/main.go @@ -14,6 +14,7 @@ import ( "os" "path/filepath" "runtime/debug" + "slices" "strings" "github.com/desertbit/columnize" @@ -318,17 +319,17 @@ func run(api DdbAPI, log *logging.LeveledLogger, opts cliOptions, parser *flags. if opts.VosPath != "" { // Commands that manage the pool open/close lifecycle themselves and must // not have the pool pre-opened by the CLI layer. - noAutoOpen := map[string]bool{ - "feature": true, - "open": true, - "close": true, - "prov_mem": true, - "smd_sync": true, - "rm_pool": true, - "dev_list": true, - "dev_replace": true, + noAutoOpen := []string{ + "feature", + "open", + "close", + "prov_mem", + "smd_sync", + "rm_pool", + "dev_list", + "dev_replace", } - if !noAutoOpen[opts.Args.RunCmd] { + if !slices.Contains(noAutoOpen, opts.Args.RunCmd) { log.Debugf("Connect to path: %s\n", opts.VosPath) if err := api.Open(string(opts.VosPath), string(opts.SysdbPath), opts.WriteMode); err != nil { return errors.Wrapf(err, vosPathOpenErr, opts.VosPath) From 425fc486449c9b41c326c2f65e0923530e320d5c Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Mon, 4 May 2026 09:19:41 +0000 Subject: [PATCH 8/9] daos-18304 ddb: capitalize VOS in help strings Update help text and descriptions to use "VOS" instead of "vos" for consistency. Also clarify "vos db" references to use "sys db" where appropriate. Signed-off-by: Cedric Koch-Hofer --- src/control/cmd/ddb/ddb_commands.go | 38 +++++++++++++-------------- src/control/cmd/ddb/main.go | 6 ++--- src/utils/ddb/tests/ddb_parse_tests.c | 2 +- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/control/cmd/ddb/ddb_commands.go b/src/control/cmd/ddb/ddb_commands.go index 28c743e5db4..a07f1a1276a 100644 --- a/src/control/cmd/ddb/ddb_commands.go +++ b/src/control/cmd/ddb/ddb_commands.go @@ -38,17 +38,17 @@ func addAppCommands(app *grumble.App, api DdbAPI) { app.AddCommand(&grumble.Command{ Name: "open", Aliases: nil, - Help: "Opens the vos file at ", - LongHelp: `Opens the vos file at . The '-w' option allows for modifying the vos file + Help: "Opens the VOS file at ", + LongHelp: `Opens the VOS file at . The '-w' option allows for modifying the VOS file with the rm, load, commit_ilog, etc commands. The path should be an absolute path to the pool shard. Part of the path is used to determine what the pool uuid is.`, HelpGroup: "vos", Flags: func(f *grumble.Flags) { - f.Bool("w", "write_mode", false, "Open the vos file in write mode.") + f.Bool("w", "write_mode", false, "Open the VOS file in write mode.") f.String("p", "db_path", "", "Path to the sys db to open.") }, Args: func(a *grumble.Args) { - a.String("path", "Path to the vos file to open.") + a.String("path", "Path to the VOS file to open.") }, Run: func(c *grumble.Context) error { return api.Open(c.Args.String("path"), c.Flags.String("db_path"), c.Flags.Bool("write_mode")) @@ -71,7 +71,7 @@ pool shard. Part of the path is used to determine what the pool uuid is.`, app.AddCommand(&grumble.Command{ Name: "close", Aliases: nil, - Help: "Close the currently opened vos pool shard", + Help: "Close the currently opened VOS pool shard", LongHelp: "", HelpGroup: "vos", Run: func(c *grumble.Context) error { @@ -96,7 +96,7 @@ pool shard. Part of the path is used to determine what the pool uuid is.`, Name: "value_dump", Aliases: nil, Help: "Dump a value", - LongHelp: `Dump a value to the screen or file. The vos path should be a complete + LongHelp: `Dump a value to the screen or file. The VOS path should be a complete path, including the akey and if the value is an array value it should include the extent. If a path to a file was provided then the value will be written to the file, else it will be printed to the screen.`, @@ -130,17 +130,17 @@ and everything under it, to a single value.`, app.AddCommand(&grumble.Command{ Name: "value_load", Aliases: nil, - Help: "Load a value to a vos path. ", - LongHelp: `Load a value to a vos path. This can be used to update + Help: "Load a value to a VOS path. ", + LongHelp: `Load a value to a VOS path. This can be used to update the value of an existing key, or create a new key. The is a path to a -file on the file system. The is a vos tree path to a value where the +file on the file system. The is a VOS tree path to a value where the data will be loaded. If the path currently exists, then the destination path must match the value type, meaning, if the value type is an array, then the path must include the extent, otherwise, it must not.`, HelpGroup: "vos", Args: func(a *grumble.Args) { a.String("src", "Source file path.") - a.String("dst", "Destination vos tree path to a value.") + a.String("dst", "Destination VOS tree path to a value.") }, Run: func(c *grumble.Context) error { return api.ValueLoad(c.Args.String("src"), c.Args.String("dst")) @@ -235,7 +235,7 @@ the path must include the extent, otherwise, it must not.`, HelpGroup: "smd", Args: func(a *grumble.Args) { a.String("nvme_conf", "Path to the nvme conf file. (default /mnt/daos/daos_nvme.conf)", grumble.Default("")) - a.String("db_path", "Path to the vos db. (default /mnt/daos)", grumble.Default("")) + a.String("db_path", "Path to the sys db. (default /mnt/daos)", grumble.Default("")) }, Run: func(c *grumble.Context) error { return api.SmdSync(c.Args.String("nvme_conf"), c.Args.String("db_path")) @@ -306,17 +306,17 @@ the path must include the extent, otherwise, it must not.`, app.AddCommand(&grumble.Command{ Name: "feature", Aliases: nil, - Help: "Manage vos pool features", + Help: "Manage VOS pool features", LongHelp: "", HelpGroup: "vos", Flags: func(f *grumble.Flags) { - f.String("e", "enable", "", "Enable vos pool features") - f.String("d", "disable", "", "Disable vos pool features") + f.String("e", "enable", "", "Enable VOS pool features") + f.String("d", "disable", "", "Disable VOS pool features") f.String("p", "db_path", "", "Path to the sys db") f.Bool("s", "show", false, "Show current features") }, Args: func(a *grumble.Args) { - a.String("path", "Optional, Path to the vos file", grumble.Default("")) + a.String("path", "Optional, Path to the VOS file", grumble.Default("")) }, Run: func(c *grumble.Context) error { return api.Feature(c.Args.String("path"), c.Flags.String("db_path"), c.Flags.String("enable"), c.Flags.String("disable"), c.Flags.Bool("show")) @@ -327,14 +327,14 @@ the path must include the extent, otherwise, it must not.`, app.AddCommand(&grumble.Command{ Name: "rm_pool", Aliases: nil, - Help: "Remove a vos pool.", + Help: "Remove a VOS pool.", LongHelp: "", HelpGroup: "vos", Flags: func(f *grumble.Flags) { f.String("p", "db_path", "", "Path to the sys db") }, Args: func(a *grumble.Args) { - a.String("path", "Path to the vos file") + a.String("path", "Path to the VOS file") }, Run: func(c *grumble.Context) error { return api.RmPool(c.Args.String("path"), c.Flags.String("db_path")) @@ -365,7 +365,7 @@ the path must include the extent, otherwise, it must not.`, LongHelp: "", HelpGroup: "vos", Args: func(a *grumble.Args) { - a.String("db_path", "Path to the vos db.") + a.String("db_path", "Path to the sys db.") }, Run: func(c *grumble.Context) error { return api.DevList(c.Args.String("db_path")) @@ -380,7 +380,7 @@ the path must include the extent, otherwise, it must not.`, LongHelp: "", HelpGroup: "vos", Args: func(a *grumble.Args) { - a.String("db_path", "Path to the vos db.") + a.String("db_path", "Path to the sys db.") a.String("old_dev", "Old device UUID.") a.String("new_dev", "New device UUID.") }, diff --git a/src/control/cmd/ddb/main.go b/src/control/cmd/ddb/main.go index fdc846aca0f..7f08e2b87c3 100644 --- a/src/control/cmd/ddb/main.go +++ b/src/control/cmd/ddb/main.go @@ -56,7 +56,7 @@ func exitWithError(err error) { } type cliOptions struct { - WriteMode bool `long:"write_mode" short:"w" description:"Open the vos file in write mode."` + WriteMode bool `long:"write_mode" short:"w" description:"Open the VOS file in write mode."` CmdFile string `long:"cmd_file" short:"f" description:"Path to a file containing a sequence of ddb commands to execute."` SysdbPath string `long:"db_path" short:"p" description:"Path to the sys db."` VosPath string `long:"vos_path" short:"s" description:"Path to the VOS file to open."` @@ -75,7 +75,7 @@ Available Commands: ` const helpTreePath = ` -Vos Tree Paths: +VOS Paths: Many of the commands take a VOS tree path. The format for this path is [cont]/[obj]/[dkey]/[akey]/[extent]. To make it easier to navigate the tree, indexes can be used @@ -104,7 +104,7 @@ MODE section of the manpage for details. const grumbleUnknownCmdErr = "unknown command, try 'help'" const runCmdArgsErr = "Cannot use both command file and a command string" -const vosPathMissErr = "Cannot use sys db path without a vos path" +const vosPathMissErr = "Cannot use sys db path without a VOS path" const loggerInitErr = "Logging facilities cannot be initialized" const ctxInitErr = "DDB Context cannot be initialized" const vosPathOpenErr = "Error opening VOS path '%s'" diff --git a/src/utils/ddb/tests/ddb_parse_tests.c b/src/utils/ddb/tests/ddb_parse_tests.c index 7db9e085a8e..8392560c6a8 100644 --- a/src/utils/ddb/tests/ddb_parse_tests.c +++ b/src/utils/ddb/tests/ddb_parse_tests.c @@ -75,7 +75,7 @@ parse_vos_file_parts_test_errors(void **state) rc = parse_vos_file_parts("/mnt/daos/" MOCKED_POOL_UUID_STR "/vos-01", NULL, &parts); assert_rc_equal(rc, -DER_INVAL); - /* Test invalid vos paths with too long vos path */ + /* Test invalid VOS paths with too long VOS path */ D_ALLOC_ARRAY_CHECK(buf, VOS_PATH_SIZE + 1); memset(buf, 'a', VOS_PATH_SIZE + 1); buf[0] = '/'; From beaeed71b4697a5347c6c5dbae6192f167208936 Mon Sep 17 00:00:00 2001 From: Cedric Koch-Hofer Date: Mon, 4 May 2026 09:31:00 +0000 Subject: [PATCH 9/9] daos-18304 ddb: improve unknown command error message Fix reviewers comments. Signed-off-by: Cedric Koch-Hofer --- src/control/cmd/ddb/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/control/cmd/ddb/main.go b/src/control/cmd/ddb/main.go index 7f08e2b87c3..3212a87d452 100644 --- a/src/control/cmd/ddb/main.go +++ b/src/control/cmd/ddb/main.go @@ -37,7 +37,7 @@ type unknownCmdError struct { } func (e *unknownCmdError) Error() string { - return fmt.Sprintf("Error running command '%s' unknown command, try 'help'", e.cmd) + return fmt.Sprintf("Unknown command: '%s'. Run 'help' to see available commands.", e.cmd) } func exitWithError(err error) {