From 1d3c9d568a5764966391699e3551a4422e55c1c0 Mon Sep 17 00:00:00 2001 From: Ryon Jensen Date: Thu, 26 May 2022 08:09:35 -0600 Subject: [PATCH 1/2] DAOS-10418 ddb: More program options - Add -w option so commands that modify the tree can only be run in "write mode". - Add testing for -R option - Fix a bug where the pool wasn't being closed on program exit. - Add -f command to pass in a file with a list of commands - Restructure some of the ddb_main code for better reuse - Don't actually need to add "program_name" back in when running commands Signed-off-by: Ryon Jensen Skip-func-test: true Signed-off-by: Ryon Jensen --- src/ddb/ddb_cmd_options.c | 30 +---- src/ddb/ddb_commands.c | 4 +- src/ddb/ddb_common.h | 18 ++- src/ddb/ddb_entry.c | 29 ++++- src/ddb/ddb_main.c | 110 ++++++++++------ src/ddb/ddb_parse.c | 10 +- src/ddb/ddb_parse.h | 2 +- src/ddb/tests/ddb_cmd_options_tests.c | 5 +- src/ddb/tests/ddb_main_tests.c | 173 ++++++++++++++++++++++---- src/ddb/tests/ddb_smoke_tests.sh | 32 ++--- src/ddb/tests/ddb_test_driver.c | 2 + src/ddb/tests/ddb_test_driver.h | 1 + 12 files changed, 285 insertions(+), 131 deletions(-) diff --git a/src/ddb/ddb_cmd_options.c b/src/ddb/ddb_cmd_options.c index 00be286213f..8e3af2ea828 100644 --- a/src/ddb/ddb_cmd_options.c +++ b/src/ddb/ddb_cmd_options.c @@ -56,9 +56,6 @@ ls_option_parse(struct ddb_ctx *ctx, struct ls_options *cmd_args, index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_LS)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -101,9 +98,6 @@ dump_value_option_parse(struct ddb_ctx *ctx, struct dump_value_options *cmd_args index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_DUMP_VALUE)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -156,9 +150,6 @@ rm_option_parse(struct ddb_ctx *ctx, struct rm_options *cmd_args, index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_RM)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -204,9 +195,6 @@ load_option_parse(struct ddb_ctx *ctx, struct load_options *cmd_args, index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_LOAD)); - index++; if (argc - index > 0) { cmd_args->src = argv[index]; index++; @@ -266,9 +254,6 @@ dump_ilog_option_parse(struct ddb_ctx *ctx, struct dump_ilog_options *cmd_args, index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_DUMP_ILOG)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -314,9 +299,6 @@ commit_ilog_option_parse(struct ddb_ctx *ctx, struct commit_ilog_options *cmd_ar index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_COMMIT_ILOG)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -362,9 +344,6 @@ rm_ilog_option_parse(struct ddb_ctx *ctx, struct rm_ilog_options *cmd_args, index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_RM_ILOG)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -418,9 +397,6 @@ dump_dtx_option_parse(struct ddb_ctx *ctx, struct dump_dtx_options *cmd_args, index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_DUMP_DTX)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -466,9 +442,6 @@ clear_cmt_dtx_option_parse(struct ddb_ctx *ctx, struct clear_cmt_dtx_options *cm index = optind; - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_CLEAR_CMT_DTX)); - index++; if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -488,8 +461,9 @@ clear_cmt_dtx_option_parse(struct ddb_ctx *ctx, struct clear_cmt_dtx_options *cm int ddb_parse_cmd_args(struct ddb_ctx *ctx, struct argv_parsed *parsed, struct ddb_cmd_info *info) { - char *cmd = parsed->ap_argv[1]; + char *cmd = parsed->ap_argv[0]; + D_ASSERT(cmd != NULL); if (same(cmd, COMMAND_NAME_HELP)) { info->dci_cmd = DDB_CMD_HELP; return 0; diff --git a/src/ddb/ddb_commands.c b/src/ddb/ddb_commands.c index 649b052103e..d9fe57e65e5 100644 --- a/src/ddb/ddb_commands.c +++ b/src/ddb/ddb_commands.c @@ -449,8 +449,10 @@ ddb_run_load(struct ddb_ctx *ctx, struct load_options *opt) if (rc < 0) { ddb_errorf(ctx, "System error: "DF_RC"\n", DP_RC(rc)); D_GOTO(done, rc); + } else if (!(rc == iov.iov_buf_len && rc == iov.iov_len)) { + D_ERROR("Bytes read from file does not match results from get file size\n"); + return -DER_UNKNOWN; } - D_ASSERT(rc == iov.iov_buf_len && rc == iov.iov_len); rc = dv_update(ctx->dc_poh, &vtpb.vtp_path, &iov, epoch); if (!SUCCESS(rc)) { diff --git a/src/ddb/ddb_common.h b/src/ddb/ddb_common.h index 21582bdd4c0..58bfce83a89 100644 --- a/src/ddb/ddb_common.h +++ b/src/ddb/ddb_common.h @@ -17,8 +17,8 @@ #define SUCCESS(rc) ((rc) == DER_SUCCESS) #define ddb_print(ctx, str) \ - do { if (ctx->dc_io_ft.ddb_print_message) \ - ctx->dc_io_ft.ddb_print_message(str); \ + do { if ((ctx)->dc_io_ft.ddb_print_message) \ + (ctx)->dc_io_ft.ddb_print_message(str); \ else \ printf(str); } while (0) @@ -43,6 +43,8 @@ } while (0) +typedef int (*ddb_io_line_cb)(void *cb_args, char *line, uint32_t str_len); + struct ddb_io_ft { /** * Print a message. @@ -102,13 +104,23 @@ struct ddb_io_ft { * @return number of bytes read from the src_path */ size_t (*ddb_read_file)(const char *src_path, d_iov_t *contents); + + /** + * Read contents of a file line by line. For each line, the line_cb will be called. + * @param path Path of the file to read + * @param line_cb Callback function used for each line + * @param cb_args Caller arguments passed to the callback function + * @return 0 on success, else an error code + */ + int (*ddb_get_lines)(const char *path, ddb_io_line_cb line_cb, void *cb_args); + }; struct ddb_ctx { struct ddb_io_ft dc_io_ft; daos_handle_t dc_poh; - daos_handle_t dc_coh; bool dc_should_quit; + bool dc_write_mode; }; struct dv_tree_path { diff --git a/src/ddb/ddb_entry.c b/src/ddb/ddb_entry.c index bf94882bd72..ac6daf7000d 100644 --- a/src/ddb/ddb_entry.c +++ b/src/ddb/ddb_entry.c @@ -89,6 +89,32 @@ file_exists(const char *path) return access(path, F_OK) == 0; } +static int +get_lines(const char *path, ddb_io_line_cb line_cb, void *cb_args) +{ + FILE *f; + char *line = NULL; + uint64_t len = 0; + uint64_t read; + int rc = 0; + + f = fopen(path, "r"); + if (f == NULL) { + rc = daos_errno2der(errno); + print_error("Unable to open path '%s': "DF_RC"\n", path, DP_RC(rc)); + return rc; + } + + while ((read = getline(&line, &len, f)) != -1 && rc == 0) + rc = line_cb(cb_args, line, read); + + fclose(f); + if (line) + free(line); + + return rc; +} + int main(int argc, char *argv[]) { int rc; @@ -99,7 +125,8 @@ int main(int argc, char *argv[]) .ddb_write_file = write_file, .ddb_read_file = read_file, .ddb_get_file_size = get_file_size, - .ddb_get_file_exists = file_exists + .ddb_get_file_exists = file_exists, + .ddb_get_lines = get_lines, }; rc = ddb_init(); diff --git a/src/ddb/ddb_main.c b/src/ddb/ddb_main.c index 8659c8fd670..220ad970a53 100644 --- a/src/ddb/ddb_main.c +++ b/src/ddb/ddb_main.c @@ -12,6 +12,8 @@ #include "ddb_vos.h" #include "ddb_cmd_options.h" +#define error_msg_write_mode_only "Can only modify the VOS tree in 'write mode'\n" + int ddb_init() { @@ -27,12 +29,23 @@ ddb_fini() } static int -run_cmd(struct ddb_ctx *ctx, struct argv_parsed *parse_args) +run_cmd(struct ddb_ctx *ctx, char *cmd_str, bool write_mode) { - struct ddb_cmd_info info = {0}; - int rc = 0; + struct argv_parsed parse_args = {0}; + struct ddb_cmd_info info = {0}; + int rc; + + /* Remove newline if needed */ + if (cmd_str[strlen(cmd_str) - 1] == '\n') + cmd_str[strlen(cmd_str) - 1] = '\0'; + + rc = ddb_str2argv_create(cmd_str, &parse_args); + if (!SUCCESS(rc)) + return rc; - ddb_parse_cmd_args(ctx, parse_args, &info); + rc = ddb_parse_cmd_args(ctx, &parse_args, &info); + if (!SUCCESS(rc)) + return rc; switch (info.dci_cmd) { case DDB_CMD_UNKNOWN: @@ -59,12 +72,22 @@ run_cmd(struct ddb_ctx *ctx, struct argv_parsed *parse_args) rc = ddb_run_dump_value(ctx, &info.dci_cmd_option.dci_dump_value); break; case DDB_CMD_RM: + if (!write_mode) { + ddb_print(ctx, error_msg_write_mode_only); + rc = -DER_INVAL; + break; + } rc = ddb_run_rm(ctx, &info.dci_cmd_option.dci_rm); break; case DDB_CMD_DUMP_DTX: rc = ddb_run_dump_dtx(ctx, &info.dci_cmd_option.dci_dump_dtx); break; case DDB_CMD_LOAD: + if (!write_mode) { + ddb_print(ctx, error_msg_write_mode_only); + rc = -DER_INVAL; + break; + } rc = ddb_run_load(ctx, &info.dci_cmd_option.dci_load); break; case DDB_CMD_COMMIT_ILOG: @@ -74,15 +97,31 @@ run_cmd(struct ddb_ctx *ctx, struct argv_parsed *parse_args) rc = ddb_run_rm_ilog(ctx, &info.dci_cmd_option.dci_rm_ilog); break; case DDB_CMD_CLEAR_CMT_DTX: + if (!write_mode) { + ddb_print(ctx, error_msg_write_mode_only); + rc = -DER_INVAL; + break; + } rc = ddb_run_clear_cmt_dtx(ctx, &info.dci_cmd_option.dci_clear_cmt_dtx); break; } - ddb_str2argv_free(parse_args); + ddb_str2argv_free(&parse_args); return rc; } +static int +process_line_cb(void *cb_args, char *line, uint32_t line_len) +{ + struct ddb_ctx *ctx = cb_args; + + ddb_printf(ctx, "Command: %s", line); + if (line_len <= 1) /* It's okay to only have a '\n', but don't try to run a command */ + return 0; + return run_cmd(ctx, line, ctx->dc_write_mode); +} + #define str_has_value(str) ((str) != NULL && strlen(str) > 0) int @@ -90,29 +129,28 @@ ddb_main(struct ddb_io_ft *io_ft, int argc, char *argv[]) { struct program_args pa = {0}; uint32_t input_buf_len = 1024; - uint32_t buf_len = input_buf_len * 2; - char *buf; char *input_buf; - struct argv_parsed parse_args = {0}; int rc = 0; struct ddb_ctx ctx = {0}; D_ASSERT(io_ft); ctx.dc_io_ft = *io_ft; - D_ALLOC(buf, buf_len + 1024); - if (buf == NULL) - return -DER_NOMEM; D_ALLOC(input_buf, input_buf_len); - if (input_buf == NULL) { - D_FREE(buf); + if (input_buf == NULL) return -DER_NOMEM; - } rc = ddb_parse_program_args(&ctx, argc, argv, &pa); if (!SUCCESS(rc)) D_GOTO(done, rc); + ctx.dc_write_mode = pa.pa_write_mode; + + if (str_has_value(pa.pa_r_cmd_run) && str_has_value(pa.pa_cmd_file)) { + ddb_print(&ctx, "Cannot use both '-R' and '-f'.\n"); + D_GOTO(done, rc = -DER_INVAL); + } + if (str_has_value(pa.pa_pool_path)) { rc = ddb_vos_pool_open(pa.pa_pool_path, &ctx.dc_poh); if (!SUCCESS(rc)) @@ -120,47 +158,39 @@ ddb_main(struct ddb_io_ft *io_ft, int argc, char *argv[]) } if (str_has_value(pa.pa_r_cmd_run)) { - /* Add program name back */ - snprintf(buf, buf_len, "%s %s", argv[0], pa.pa_r_cmd_run); - rc = ddb_str2argv_create(buf, &parse_args); - if (!SUCCESS(rc)) { - ddb_vos_pool_close(ctx.dc_poh); - D_GOTO(done, rc); - } - - rc = run_cmd(&ctx, &parse_args); - - ddb_str2argv_free(&parse_args); + rc = run_cmd(&ctx, pa.pa_r_cmd_run, pa.pa_write_mode); + if (!SUCCESS(rc)) + D_ERROR("Command '%s' failed: "DF_RC"\n", input_buf, DP_RC(rc)); D_GOTO(done, rc); } if (str_has_value(pa.pa_cmd_file)) { - /* Still to be implemented */ - D_GOTO(done, rc = -DER_NOSYS); + if (!io_ft->ddb_get_file_exists(pa.pa_cmd_file)) { + ddb_errorf(&ctx, "Unable to access file: '%s'\n", pa.pa_cmd_file); + D_GOTO(done, rc = -DER_INVAL); + } + + rc = io_ft->ddb_get_lines(pa.pa_cmd_file, process_line_cb, &ctx); + D_GOTO(done, rc); } while (!ctx.dc_should_quit) { io_ft->ddb_print_message("$ "); io_ft->ddb_get_input(input_buf, input_buf_len); - input_buf[strlen(input_buf) - 1] = '\0'; /* Remove newline */ - /* add program name to beginning of string that will be parsed into argv. That way - * is the same as argv from command line into main() - */ - snprintf(buf, buf_len, "%s %s", argv[0], input_buf); - rc = ddb_str2argv_create(buf, &parse_args); + rc = run_cmd(&ctx, input_buf, pa.pa_write_mode); if (!SUCCESS(rc)) { - ddb_errorf(&ctx, "Error with input: "DF_RC"\n", DP_RC(rc)); - ddb_str2argv_free(&parse_args); - continue; + D_ERROR("Command '%s' failed: "DF_RC"\n", input_buf, DP_RC(rc)); } - - rc = run_cmd(&ctx, &parse_args); - ddb_str2argv_free(&parse_args); } done: - D_FREE(buf); + if (daos_handle_is_valid(ctx.dc_poh)) { + int tmp_rc = ddb_vos_pool_close(ctx.dc_poh); + + if (rc == 0) + rc = tmp_rc; + } D_FREE(input_buf); return rc; diff --git a/src/ddb/ddb_parse.c b/src/ddb/ddb_parse.c index 78f6e9bb063..61caa6866a8 100644 --- a/src/ddb/ddb_parse.c +++ b/src/ddb/ddb_parse.c @@ -47,17 +47,16 @@ ddb_parse_program_args(struct ddb_ctx *ctx, uint32_t argc, char **argv, struct p { "write_mode", no_argument, NULL, 'w' }, { "run_cmd", required_argument, NULL, 'R' }, { "cmd_file", required_argument, NULL, 'f' }, - { "pool", required_argument, NULL, 'p' }, { NULL } }; int index = 0, opt; optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, - "wR:f:p:", program_options, &index)) != -1) { + while ((opt = getopt_long(argc, argv, "wR:f:", program_options, &index)) != -1) { switch (opt) { case 'w': + pa->pa_write_mode = true; break; case 'R': pa->pa_r_cmd_run = optarg; @@ -65,11 +64,8 @@ ddb_parse_program_args(struct ddb_ctx *ctx, uint32_t argc, char **argv, struct p case 'f': pa->pa_cmd_file = optarg; break; - case 'p': - pa->pa_pool_uuid = optarg; - break; case '?': - ddb_errorf(ctx, "'%c' is unknown\n", optopt); + ddb_errorf(ctx, "'%c'(0x%x) is unknown\n", optopt, optopt); return -DER_INVAL; default: return -DER_INVAL; diff --git a/src/ddb/ddb_parse.h b/src/ddb/ddb_parse.h index d7d5bd023e9..be14384b2da 100644 --- a/src/ddb/ddb_parse.h +++ b/src/ddb/ddb_parse.h @@ -18,7 +18,7 @@ struct program_args { char *pa_cmd_file; char *pa_r_cmd_run; char *pa_pool_path; - char *pa_pool_uuid; + bool pa_write_mode; }; /* Parse a string into an array of words with the count of words */ diff --git a/src/ddb/tests/ddb_cmd_options_tests.c b/src/ddb/tests/ddb_cmd_options_tests.c index e87f4ba0422..e0178714be7 100644 --- a/src/ddb/tests/ddb_cmd_options_tests.c +++ b/src/ddb/tests/ddb_cmd_options_tests.c @@ -12,10 +12,9 @@ #include "ddb_test_driver.h" #define test_run_inval_cmd(...) \ - assert_rc_equal(-DER_INVAL, __test_run_cmd(NULL, (char *[]){"prog_name", \ - __VA_ARGS__, NULL})) + assert_rc_equal(-DER_INVAL, __test_run_cmd(NULL, (char *[]){__VA_ARGS__, NULL})) #define test_run_cmd(ctx, ...) \ - assert_success(__test_run_cmd(ctx, (char *[]){"prog_name", __VA_ARGS__, NULL})) + assert_success(__test_run_cmd(ctx, (char *[]){__VA_ARGS__, NULL})) static int fake_print(const char *fmt, ...) diff --git a/src/ddb/tests/ddb_main_tests.c b/src/ddb/tests/ddb_main_tests.c index dd42584f64f..0fafaeaf02f 100644 --- a/src/ddb/tests/ddb_main_tests.c +++ b/src/ddb/tests/ddb_main_tests.c @@ -6,6 +6,7 @@ #include #include #include +#include #include "ddb_cmocka.h" #include "ddb_test_driver.h" @@ -14,19 +15,6 @@ * command line options and arguments are handled correctly and the interactive mode. */ - -#define assert_main(...) \ - assert_success(__test_run_main((char *[]){"prog_name", __VA_ARGS__, NULL})) -#define assert_invalid_main(...) \ - assert_rc_equal(-DER_INVAL, __test_run_main((char *[]){"prog_name", __VA_ARGS__, NULL})) - - -static int -fake_print_message(const char *fmt, ...) -{ - return 0; -} - uint32_t fake_get_input_called; int fake_get_input_inputs_count; int fake_get_input_inputs_idx; @@ -62,14 +50,43 @@ fake_get_input(char *buf, uint32_t buf_len) return input; } +int dvt_fake_get_lines_result; +int dvt_fake_get_lines_called; +int +dvt_fake_get_lines(const char *path, ddb_io_line_cb line_cb, void *cb_args) +{ + int i; + int rc; + + dvt_fake_get_lines_called++; + + for (i = 0; i < fake_get_input_inputs_count; i++) { + rc = line_cb(cb_args, fake_get_input_inputs[i], strlen(fake_get_input_inputs[i])); + if (rc != 0) + return rc; + } + + + return dvt_fake_get_lines_result; +} + +#define assert_main(...) \ + assert_success(__test_run_main((char *[]){"prog_name", __VA_ARGS__, NULL})) +#define assert_invalid_main(...) \ + assert_rc_equal(-DER_INVAL, __test_run_main((char *[]){"prog_name", __VA_ARGS__, NULL})) + static int __test_run_main(char *argv[]) { uint32_t argc = 0; struct ddb_io_ft ft = { - .ddb_print_message = fake_print_message, - .ddb_print_error = fake_print_message, + .ddb_print_message = dvt_fake_print, + .ddb_print_error = dvt_fake_print, .ddb_get_input = fake_get_input, + .ddb_read_file = dvt_fake_read_file, + .ddb_get_file_exists = dvt_fake_get_file_exists, + .ddb_get_file_size = dvt_fake_get_file_size, + .ddb_get_lines = dvt_fake_get_lines }; assert_non_null(argv); @@ -114,12 +131,119 @@ interactive_mode_tests(void **state) assert_invalid_main("path", "invalid_extra_arg"); } -/* Not tested yet: - * - -R is passed - * - -f is passed - * - -w is passed - * - pool shard file is passed as argument - */ +static void +run_inline_command_with_opt_r_tests(void **state) +{ + struct dt_vos_pool_ctx *tctx = *state; + + assert_main(tctx->dvt_pmem_file, "-R", "ls [0] -r"); +} + +static void +only_modify_with_option_w_tests(void **state) +{ + struct dt_vos_pool_ctx *tctx = *state; + +#define assert_requires_write_mode(cmd) \ +do { \ + assert_invalid_main(tctx->dvt_pmem_file, "-R", cmd); \ + assert_main(tctx->dvt_pmem_file, "-w", "-R", cmd); \ +} while (0) + + dvt_fake_print_reset(); + assert_requires_write_mode("rm [0]"); + + /* Set up test for the load command */ + dvt_fake_get_file_exists_result = true; + dvt_fake_get_file_size_result = 10; + dvt_fake_read_file_result = dvt_fake_get_file_size_result; + assert_requires_write_mode("load src [0]/[0]/[0]/[1] 1"); + + assert_requires_write_mode("clear_cmt_dtx [0]"); +} + +static void +run_many_commands_with_option_f_tests(void **state) +{ + struct dt_vos_pool_ctx *tctx = *state; + + /* file doesn't exist */ + dvt_fake_get_file_exists_result = false; + assert_invalid_main(tctx->dvt_pmem_file, "-f", "file_path"); + + /* Empty file is still success */ + dvt_fake_get_file_exists_result = true; + assert_main(tctx->dvt_pmem_file, "-f", "file_path"); + + /* one command */ + dvt_fake_get_lines_called = 0; + assert_main(tctx->dvt_pmem_file, "-f", "file_path"); + assert_int_equal(1, dvt_fake_get_lines_called); + + /* handles invalid commands */ + dvt_fake_get_file_exists_result = true; + set_fake_inputs("bad_command"); + assert_invalid_main(tctx->dvt_pmem_file, "-f", "file_path"); + + /* multiple lines/commands */ + dvt_fake_get_file_exists_result = true; + dvt_fake_get_lines_called = 0; + set_fake_inputs("ls", "dump_superblock", "ls [0]"); + assert_main(tctx->dvt_pmem_file, "-f", "file_path"); + assert_int_equal(1, dvt_fake_get_lines_called); + + /* empty lines are ignored */ + dvt_fake_get_file_exists_result = true; + dvt_fake_get_lines_called = 0; + set_fake_inputs("ls", "", "dump_superblock"); + assert_main(tctx->dvt_pmem_file, "-f", "file_path"); + assert_int_equal(1, dvt_fake_get_lines_called); + + /* commands that modify tree must have '-w' also */ + dvt_fake_get_file_exists_result = true; + set_fake_inputs("ls", "rm [0]"); + assert_invalid_main(tctx->dvt_pmem_file, "-f", "file_path"); + assert_main(tctx->dvt_pmem_file, "-w", "-f", "file_path"); +} + +static void +option_f_and_option_R_is_invalid_tests(void **state) +{ + struct dt_vos_pool_ctx *tctx = *state; + + /* Make sure that the fakes are setup to work so they are not invalid */ + set_fake_inputs("ls"); + dvt_fake_get_file_exists_result = true; + + assert_invalid_main(tctx->dvt_pmem_file, "-R", "ls", "-f", "file_path"); +} + +static int +ddb_main_suit_setup(void **state) +{ + struct dt_vos_pool_ctx *tctx; + + 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(vos_pool_open(tctx->dvt_pmem_file, tctx->dvt_pool_uuid, 0, &tctx->dvt_poh)); + + return 0; +} + +static int +ddb_main_suit_teardown(void **state) +{ + struct dt_vos_pool_ctx *tctx = *state; + + if (tctx == NULL) + fail_msg("Test not setup correctly"); + assert_success(vos_pool_close(tctx->dvt_poh)); + ddb_teardown_vos(state); + + return 0; +} #define TEST(x) { #x, x, NULL, NULL } int @@ -127,7 +251,12 @@ ddb_main_tests() { static const struct CMUnitTest tests[] = { TEST(interactive_mode_tests), + TEST(run_inline_command_with_opt_r_tests), + TEST(only_modify_with_option_w_tests), + TEST(run_many_commands_with_option_f_tests), + TEST(option_f_and_option_R_is_invalid_tests), }; - return cmocka_run_group_tests_name("DDB CLI tests", tests, NULL, NULL); + return cmocka_run_group_tests_name("DDB CLI tests", tests, ddb_main_suit_setup, + ddb_main_suit_teardown); } diff --git a/src/ddb/tests/ddb_smoke_tests.sh b/src/ddb/tests/ddb_smoke_tests.sh index 95d4d04c0fe..e6616038e3d 100755 --- a/src/ddb/tests/ddb_smoke_tests.sh +++ b/src/ddb/tests/ddb_smoke_tests.sh @@ -66,29 +66,11 @@ run_cmd ddb $vos_file -R "dump_value $vos_path /tmp/ddb_value_dump" run_cmd cat /tmp/ddb_value_dump run_cmd diff /tmp/ddb_new_value /tmp/ddb_value_dump -msg "'load' to new key" -vos_path="[0]/[0]/[0]/\'new_new_new_new_key\'" -run_cmd ddb $vos_file -R "load /tmp/ddb_new_value $vos_path 1" -run_cmd ddb $vos_file -R "dump_value $vos_path /tmp/ddb_value_dump" -run_cmd cat /tmp/ddb_value_dump -run_cmd ddb $vos_file -R 'ls [0]/[0]/[0] -r' -diff /tmp/ddb_new_value /tmp/ddb_value_dump - -msg "'rm'" -run_cmd ddb $vos_file -R 'ls' -run_cmd ddb $vos_file -R 'rm [1]' -run_cmd ddb $vos_file -R 'ls' - - -msg "'superblock', 'ilog' and 'dtx' dumps" -run_cmd ddb $vos_file -R 'dump_superblock' -run_cmd ddb $vos_file -R 'dump_ilog [0]/[0]' -run_cmd ddb $vos_file -R 'dump_ilog [0]/[0]/[0]' -run_cmd ddb $vos_file -R 'dump_ilog [0]/[0]/[0]/[0]' -run_cmd ddb $vos_file -R 'commit_ilog [0]/[0]/[0]/[0]' -run_cmd ddb $vos_file -R 'rm_ilog [1]/[0]/[0]' - -run_cmd ddb $vos_file -R 'dump_dtx [0]' -run_cmd ddb $vos_file -R 'clear_cmt_dtx [0]' -run_cmd ddb $vos_file -R 'dump_dtx [0]' +rm -f /tmp/ddb_commands +touch /tmp/ddb_commands +echo "ls" >> /tmp/ddb_commands +echo "ls [0]" >> /tmp/ddb_commands +echo "ls [0]/[0]" >> /tmp/ddb_commands +echo "dump_superblock" >> /tmp/ddb_commands +run_cmd ddb $vos_file -f /tmp/ddb_commands diff --git a/src/ddb/tests/ddb_test_driver.c b/src/ddb/tests/ddb_test_driver.c index f66efeffd7c..07323a2a656 100644 --- a/src/ddb/tests/ddb_test_driver.c +++ b/src/ddb/tests/ddb_test_driver.c @@ -175,6 +175,7 @@ dvt_fake_get_file_exists(const char *path) return dvt_fake_get_file_exists_result; } +uint32_t dvt_fake_read_file_called; size_t dvt_fake_read_file_result; char dvt_fake_read_file_buf[64]; @@ -183,6 +184,7 @@ dvt_fake_read_file(const char *src_path, d_iov_t *contents) { size_t to_copy = min(contents->iov_buf_len, ARRAY_SIZE(dvt_fake_read_file_buf)); + dvt_fake_read_file_called++; memcpy(contents->iov_buf, dvt_fake_read_file_buf, to_copy); contents->iov_len = to_copy; diff --git a/src/ddb/tests/ddb_test_driver.h b/src/ddb/tests/ddb_test_driver.h index bcdca0b2e87..1f9cc5640e8 100644 --- a/src/ddb/tests/ddb_test_driver.h +++ b/src/ddb/tests/ddb_test_driver.h @@ -69,6 +69,7 @@ size_t dvt_fake_get_file_size(const char *path); extern bool dvt_fake_get_file_exists_result; bool dvt_fake_get_file_exists(const char *path); +extern uint32_t dvt_fake_read_file_called; extern size_t dvt_fake_read_file_result; extern char dvt_fake_read_file_buf[64]; size_t dvt_fake_read_file(const char *src_path, d_iov_t *contents); From c1e7dfb558da39a2b7a23d2799d37d2638f1e7a5 Mon Sep 17 00:00:00 2001 From: Ryon Jensen Date: Fri, 3 Jun 2022 06:15:41 -0600 Subject: [PATCH 2/2] Review Feedback and valgrind issue fixed Signed-off-by: Ryon Jensen Skip-func-test: true --- src/ddb/ddb_cmd_options.c | 175 +++++++++----------------- src/ddb/ddb_cmd_options.h | 2 +- src/ddb/ddb_commands.c | 2 +- src/ddb/ddb_entry.c | 13 +- src/ddb/ddb_main.c | 54 ++++++-- src/ddb/ddb_parse.c | 3 +- src/ddb/tests/ddb_cmd_options_tests.c | 2 +- src/ddb/tests/ddb_main_tests.c | 7 ++ 8 files changed, 124 insertions(+), 134 deletions(-) diff --git a/src/ddb/ddb_cmd_options.c b/src/ddb/ddb_cmd_options.c index e11c284fd78..c7e8bf26acf 100644 --- a/src/ddb/ddb_cmd_options.c +++ b/src/ddb/ddb_cmd_options.c @@ -29,12 +29,10 @@ /* Parse command line options for the 'ls' command */ static int ls_option_parse(struct ddb_ctx *ctx, struct ls_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = "r"; int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; struct option options_long[] = { { "recursive", no_argument, NULL, 'r' }, { NULL } @@ -58,7 +56,6 @@ ls_option_parse(struct ddb_ctx *ctx, struct ls_options *cmd_args, } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -75,12 +72,10 @@ ls_option_parse(struct ddb_ctx *ctx, struct ls_options *cmd_args, /* Parse command line options for the 'open' command */ static int open_option_parse(struct ddb_ctx *ctx, struct open_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = "w"; int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; struct option options_long[] = { { "write_mode", no_argument, NULL, 'w' }, { NULL } @@ -104,10 +99,6 @@ open_option_parse(struct ddb_ctx *ctx, struct open_options *cmd_args, } index = optind; - - D_ASSERT(argc > index); - D_ASSERT(same(argv[index], COMMAND_NAME_OPEN)); - index++; if (argc - index > 0) { cmd_args->vos_pool_shard = argv[index]; index++; @@ -127,12 +118,10 @@ open_option_parse(struct ddb_ctx *ctx, struct open_options *cmd_args, /* Parse command line options for the 'dump_value' command */ static int dump_value_option_parse(struct ddb_ctx *ctx, struct dump_value_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -142,17 +131,12 @@ dump_value_option_parse(struct ddb_ctx *ctx, struct dump_value_options *cmd_args /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -179,12 +163,10 @@ dump_value_option_parse(struct ddb_ctx *ctx, struct dump_value_options *cmd_args /* Parse command line options for the 'rm' command */ static int rm_option_parse(struct ddb_ctx *ctx, struct rm_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -194,17 +176,12 @@ rm_option_parse(struct ddb_ctx *ctx, struct rm_options *cmd_args, /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -224,12 +201,10 @@ rm_option_parse(struct ddb_ctx *ctx, struct rm_options *cmd_args, /* Parse command line options for the 'load' command */ static int load_option_parse(struct ddb_ctx *ctx, struct load_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -239,17 +214,12 @@ load_option_parse(struct ddb_ctx *ctx, struct load_options *cmd_args, /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->src = argv[index]; index++; @@ -283,12 +253,10 @@ load_option_parse(struct ddb_ctx *ctx, struct load_options *cmd_args, /* Parse command line options for the 'dump_ilog' command */ static int dump_ilog_option_parse(struct ddb_ctx *ctx, struct dump_ilog_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -298,17 +266,12 @@ dump_ilog_option_parse(struct ddb_ctx *ctx, struct dump_ilog_options *cmd_args, /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -328,12 +291,10 @@ dump_ilog_option_parse(struct ddb_ctx *ctx, struct dump_ilog_options *cmd_args, /* Parse command line options for the 'commit_ilog' command */ static int commit_ilog_option_parse(struct ddb_ctx *ctx, struct commit_ilog_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -343,17 +304,12 @@ commit_ilog_option_parse(struct ddb_ctx *ctx, struct commit_ilog_options *cmd_ar /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -373,12 +329,10 @@ commit_ilog_option_parse(struct ddb_ctx *ctx, struct commit_ilog_options *cmd_ar /* Parse command line options for the 'rm_ilog' command */ static int rm_ilog_option_parse(struct ddb_ctx *ctx, struct rm_ilog_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -388,17 +342,12 @@ rm_ilog_option_parse(struct ddb_ctx *ctx, struct rm_ilog_options *cmd_args, /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -418,12 +367,10 @@ rm_ilog_option_parse(struct ddb_ctx *ctx, struct rm_ilog_options *cmd_args, /* Parse command line options for the 'dump_dtx' command */ static int dump_dtx_option_parse(struct ddb_ctx *ctx, struct dump_dtx_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = "ac"; int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; struct option options_long[] = { { "active", no_argument, NULL, 'a' }, { "committed", no_argument, NULL, 'c' }, @@ -451,7 +398,6 @@ dump_dtx_option_parse(struct ddb_ctx *ctx, struct dump_dtx_options *cmd_args, } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -471,12 +417,10 @@ dump_dtx_option_parse(struct ddb_ctx *ctx, struct dump_dtx_options *cmd_args, /* Parse command line options for the 'clear_cmt_dtx' command */ static int clear_cmt_dtx_option_parse(struct ddb_ctx *ctx, struct clear_cmt_dtx_options *cmd_args, - struct argv_parsed *argc_v) + uint32_t argc, char **argv) { char *options_short = ""; - int index = 0, opt; - uint32_t argc = argc_v->ap_argc; - char **argv = argc_v->ap_argv; + int index = 0; struct option options_long[] = { { NULL } }; @@ -486,17 +430,12 @@ clear_cmt_dtx_option_parse(struct ddb_ctx *ctx, struct clear_cmt_dtx_options *cm /* Restart getopt */ optind = 1; opterr = 0; - while ((opt = getopt_long(argc, argv, options_short, options_long, &index)) != -1) { - switch (opt) { - case '?': - ddb_printf(ctx, "Unknown option: '%c'\n", optopt); - default: - return -DER_INVAL; - } + if (getopt_long(argc, argv, options_short, options_long, &index) != -1) { + ddb_printf(ctx, "Unknown option: '%c'\n", optopt); + return -DER_INVAL; } index = optind; - if (argc - index > 0) { cmd_args->path = argv[index]; index++; @@ -514,11 +453,10 @@ clear_cmt_dtx_option_parse(struct ddb_ctx *ctx, struct clear_cmt_dtx_options *cm } int -ddb_parse_cmd_args(struct ddb_ctx *ctx, struct argv_parsed *parsed, struct ddb_cmd_info *info) +ddb_parse_cmd_args(struct ddb_ctx *ctx, uint32_t argc, char **argv, struct ddb_cmd_info *info) { - char *cmd = parsed->ap_argv[0]; + char *cmd = argv[0]; - D_ASSERT(cmd != NULL); if (same(cmd, COMMAND_NAME_HELP)) { info->dci_cmd = DDB_CMD_HELP; return 0; @@ -529,11 +467,13 @@ ddb_parse_cmd_args(struct ddb_ctx *ctx, struct argv_parsed *parsed, struct ddb_c } if (same(cmd, COMMAND_NAME_LS)) { info->dci_cmd = DDB_CMD_LS; - return ls_option_parse(ctx, &info->dci_cmd_option.dci_ls, parsed); + return ls_option_parse(ctx, &info->dci_cmd_option.dci_ls, + argc, argv); } if (same(cmd, COMMAND_NAME_OPEN)) { info->dci_cmd = DDB_CMD_OPEN; - return open_option_parse(ctx, &info->dci_cmd_option.dci_open, parsed); + return open_option_parse(ctx, &info->dci_cmd_option.dci_open, + argc, argv); } if (same(cmd, COMMAND_NAME_CLOSE)) { info->dci_cmd = DDB_CMD_CLOSE; @@ -545,36 +485,43 @@ ddb_parse_cmd_args(struct ddb_ctx *ctx, struct argv_parsed *parsed, struct ddb_c } if (same(cmd, COMMAND_NAME_DUMP_VALUE)) { info->dci_cmd = DDB_CMD_DUMP_VALUE; - return dump_value_option_parse(ctx, &info->dci_cmd_option.dci_dump_value, parsed); + return dump_value_option_parse(ctx, &info->dci_cmd_option.dci_dump_value, + argc, argv); } if (same(cmd, COMMAND_NAME_RM)) { info->dci_cmd = DDB_CMD_RM; - return rm_option_parse(ctx, &info->dci_cmd_option.dci_rm, parsed); + return rm_option_parse(ctx, &info->dci_cmd_option.dci_rm, + argc, argv); } if (same(cmd, COMMAND_NAME_LOAD)) { info->dci_cmd = DDB_CMD_LOAD; - return load_option_parse(ctx, &info->dci_cmd_option.dci_load, parsed); + return load_option_parse(ctx, &info->dci_cmd_option.dci_load, + argc, argv); } if (same(cmd, COMMAND_NAME_DUMP_ILOG)) { info->dci_cmd = DDB_CMD_DUMP_ILOG; - return dump_ilog_option_parse(ctx, &info->dci_cmd_option.dci_dump_ilog, parsed); + return dump_ilog_option_parse(ctx, &info->dci_cmd_option.dci_dump_ilog, + argc, argv); } if (same(cmd, COMMAND_NAME_COMMIT_ILOG)) { info->dci_cmd = DDB_CMD_COMMIT_ILOG; - return commit_ilog_option_parse(ctx, &info->dci_cmd_option.dci_commit_ilog, parsed); + return commit_ilog_option_parse(ctx, &info->dci_cmd_option.dci_commit_ilog, + argc, argv); } if (same(cmd, COMMAND_NAME_RM_ILOG)) { info->dci_cmd = DDB_CMD_RM_ILOG; - return rm_ilog_option_parse(ctx, &info->dci_cmd_option.dci_rm_ilog, parsed); + return rm_ilog_option_parse(ctx, &info->dci_cmd_option.dci_rm_ilog, + argc, argv); } if (same(cmd, COMMAND_NAME_DUMP_DTX)) { info->dci_cmd = DDB_CMD_DUMP_DTX; - return dump_dtx_option_parse(ctx, &info->dci_cmd_option.dci_dump_dtx, parsed); + return dump_dtx_option_parse(ctx, &info->dci_cmd_option.dci_dump_dtx, + argc, argv); } if (same(cmd, COMMAND_NAME_CLEAR_CMT_DTX)) { info->dci_cmd = DDB_CMD_CLEAR_CMT_DTX; return clear_cmt_dtx_option_parse(ctx, &info->dci_cmd_option.dci_clear_cmt_dtx, - parsed); + argc, argv); } if (same(cmd, COMMAND_NAME_SMD_SYNC)) { info->dci_cmd = DDB_CMD_SMD_SYNC; @@ -609,10 +556,10 @@ ddb_run_help(struct ddb_ctx *ctx) ddb_print(ctx, " rm Remove a branch of the VOS tree\n"); ddb_print(ctx, " load Load an updated or new value\n"); ddb_print(ctx, " dump_ilog Dump the ilog\n"); - ddb_print(ctx, " commit_ilog Process the ilog\n"); + ddb_print(ctx, " commit_ilog Process the ilog\n"); ddb_print(ctx, " rm_ilog Remove all the ilog entries\n"); ddb_print(ctx, " dump_dtx Dump the dtx tables\n"); - ddb_print(ctx, " clear_cmt_dtx Clear the dtx committed table\n"); + ddb_print(ctx, " clear_cmt_dtx Clear the dtx committed table\n"); ddb_print(ctx, " smd_sync Restore the SMD file with backup from blob\n"); return 0; diff --git a/src/ddb/ddb_cmd_options.h b/src/ddb/ddb_cmd_options.h index 51f170c86d4..35021dbee64 100644 --- a/src/ddb/ddb_cmd_options.h +++ b/src/ddb/ddb_cmd_options.h @@ -90,7 +90,7 @@ struct ddb_cmd_info { } dci_cmd_option; }; -int ddb_parse_cmd_args(struct ddb_ctx *ctx, struct argv_parsed *parsed, struct ddb_cmd_info *info); +int ddb_parse_cmd_args(struct ddb_ctx *ctx, uint32_t argc, char **argv, struct ddb_cmd_info *info); /* Run commands ... */ int ddb_run_help(struct ddb_ctx *ctx); diff --git a/src/ddb/ddb_commands.c b/src/ddb/ddb_commands.c index a7df0890e7b..3548cd389b2 100644 --- a/src/ddb/ddb_commands.c +++ b/src/ddb/ddb_commands.c @@ -475,7 +475,7 @@ ddb_run_load(struct ddb_ctx *ctx, struct load_options *opt) D_GOTO(done, rc); } else if (!(rc == iov.iov_buf_len && rc == iov.iov_len)) { D_ERROR("Bytes read from file does not match results from get file size\n"); - return -DER_UNKNOWN; + D_GOTO(done, rc = -DER_UNKNOWN); } rc = dv_update(ctx->dc_poh, &pb.vtp_path, &iov, epoch); diff --git a/src/ddb/ddb_entry.c b/src/ddb/ddb_entry.c index ac6daf7000d..4db2f48a333 100644 --- a/src/ddb/ddb_entry.c +++ b/src/ddb/ddb_entry.c @@ -105,12 +105,21 @@ get_lines(const char *path, ddb_io_line_cb line_cb, void *cb_args) return rc; } - while ((read = getline(&line, &len, f)) != -1 && rc == 0) + while ((read = getline(&line, &len, f)) != -1) { rc = line_cb(cb_args, line, read); + if (!SUCCESS(rc)) { + print_error("Issue with line '%s': "DF_RC"\n", line, DP_RC(rc)); + break; + } + } + + rc = daos_errno2der(errno); + if (!SUCCESS(rc)) + print_error("Error reading line from file '%s': "DF_RC"\n", path, DP_RC(rc)); fclose(f); if (line) - free(line); + D_FREE(line); return rc; } diff --git a/src/ddb/ddb_main.c b/src/ddb/ddb_main.c index ccd99b7ec91..9b336a81eb3 100644 --- a/src/ddb/ddb_main.c +++ b/src/ddb/ddb_main.c @@ -29,23 +29,29 @@ ddb_fini() } static int -run_cmd(struct ddb_ctx *ctx, char *cmd_str, bool write_mode) +run_cmd(struct ddb_ctx *ctx, const char *cmd_str, bool write_mode) { - struct argv_parsed parse_args = {0}; - struct ddb_cmd_info info = {0}; - int rc; + struct argv_parsed parse_args = {0}; + struct ddb_cmd_info info = {0}; + int rc; + char *cmd_copy = strdup(cmd_str); /* Remove newline if needed */ - if (cmd_str[strlen(cmd_str) - 1] == '\n') - cmd_str[strlen(cmd_str) - 1] = '\0'; + if (cmd_copy[strlen(cmd_copy) - 1] == '\n') + cmd_copy[strlen(cmd_copy) - 1] = '\0'; - rc = ddb_str2argv_create(cmd_str, &parse_args); + rc = ddb_str2argv_create(cmd_copy, &parse_args); if (!SUCCESS(rc)) - return rc; + D_GOTO(done, rc); + + if (parse_args.ap_argc == 0) { + D_ERROR("Nothing parsed\n"); + return -DER_INVAL; + } - rc = ddb_parse_cmd_args(ctx, &parse_args, &info); + rc = ddb_parse_cmd_args(ctx, parse_args.ap_argc, parse_args.ap_argv, &info); if (!SUCCESS(rc)) - return rc; + D_GOTO(done, rc); switch (info.dci_cmd) { case DDB_CMD_UNKNOWN: @@ -78,6 +84,7 @@ run_cmd(struct ddb_ctx *ctx, char *cmd_str, bool write_mode) rc = ddb_run_dump_value(ctx, &info.dci_cmd_option.dci_dump_value); break; case DDB_CMD_RM: + /* The 'rm' command deletes branches of the tree */ if (!write_mode) { ddb_print(ctx, error_msg_write_mode_only); rc = -DER_INVAL; @@ -89,6 +96,10 @@ run_cmd(struct ddb_ctx *ctx, char *cmd_str, bool write_mode) rc = ddb_run_dump_dtx(ctx, &info.dci_cmd_option.dci_dump_dtx); break; case DDB_CMD_LOAD: + /* + * The load command loads alters the tree by modifying an existing value or + * creating a new one. + */ if (!write_mode) { ddb_print(ctx, error_msg_write_mode_only); rc = -DER_INVAL; @@ -103,6 +114,9 @@ run_cmd(struct ddb_ctx *ctx, char *cmd_str, bool write_mode) rc = ddb_run_rm_ilog(ctx, &info.dci_cmd_option.dci_rm_ilog); break; case DDB_CMD_CLEAR_CMT_DTX: + /* + * The clear_cmt_dtx command removes dtx entries. + */ if (!write_mode) { ddb_print(ctx, error_msg_write_mode_only); rc = -DER_INVAL; @@ -114,19 +128,33 @@ run_cmd(struct ddb_ctx *ctx, char *cmd_str, bool write_mode) rc = ddb_run_smd_sync(ctx); break; } - +done: ddb_str2argv_free(&parse_args); + D_FREE(cmd_copy); return rc; } +static bool +all_whitespace(const char *str, uint32_t str_len) +{ + int i; + + for (i = 0; i < str_len; i++) { + if (!isspace(str[i])) + return false; + } + return true; +} + static int process_line_cb(void *cb_args, char *line, uint32_t line_len) { struct ddb_ctx *ctx = cb_args; ddb_printf(ctx, "Command: %s", line); - if (line_len <= 1) /* It's okay to only have a '\n', but don't try to run a command */ + /* ignore empty lines */ + if (all_whitespace(line, line_len)) return 0; return run_cmd(ctx, line, ctx->dc_write_mode); } @@ -195,7 +223,7 @@ ddb_main(struct ddb_io_ft *io_ft, int argc, char *argv[]) done: if (daos_handle_is_valid(ctx.dc_poh)) { - int tmp_rc = ddb_vos_pool_close(ctx.dc_poh); + int tmp_rc = dv_pool_close(ctx.dc_poh); if (rc == 0) rc = tmp_rc; diff --git a/src/ddb/ddb_parse.c b/src/ddb/ddb_parse.c index 1d0da034106..79a0bae633e 100644 --- a/src/ddb/ddb_parse.c +++ b/src/ddb/ddb_parse.c @@ -104,7 +104,7 @@ ddb_parse_program_args(struct ddb_ctx *ctx, uint32_t argc, char **argv, struct p }; int index = 0, opt; - optind = 1; + optind = 0; /* Reinitialize getopt */ opterr = 0; while ((opt = getopt_long(argc, argv, "wR:f:", program_options, &index)) != -1) { switch (opt) { @@ -119,7 +119,6 @@ ddb_parse_program_args(struct ddb_ctx *ctx, uint32_t argc, char **argv, struct p break; case '?': ddb_errorf(ctx, "'%c'(0x%x) is unknown\n", optopt, optopt); - return -DER_INVAL; default: return -DER_INVAL; } diff --git a/src/ddb/tests/ddb_cmd_options_tests.c b/src/ddb/tests/ddb_cmd_options_tests.c index 569eea8682f..bc333322e0f 100644 --- a/src/ddb/tests/ddb_cmd_options_tests.c +++ b/src/ddb/tests/ddb_cmd_options_tests.c @@ -50,7 +50,7 @@ __test_run_cmd(struct ddb_cmd_info *info, char *argv[]) parse_args.ap_argv = argv; parse_args.ap_argc = argc; - rc = ddb_parse_cmd_args(&ctx, &parse_args, info); + rc = ddb_parse_cmd_args(&ctx, parse_args.ap_argc, parse_args.ap_argv, info); if (!SUCCESS(rc)) return rc; diff --git a/src/ddb/tests/ddb_main_tests.c b/src/ddb/tests/ddb_main_tests.c index 0fafaeaf02f..4d0251e62ab 100644 --- a/src/ddb/tests/ddb_main_tests.c +++ b/src/ddb/tests/ddb_main_tests.c @@ -199,6 +199,13 @@ run_many_commands_with_option_f_tests(void **state) assert_main(tctx->dvt_pmem_file, "-f", "file_path"); assert_int_equal(1, dvt_fake_get_lines_called); + /* Lines with just whitespace are ignored */ + dvt_fake_get_file_exists_result = true; + dvt_fake_get_lines_called = 0; + set_fake_inputs("ls", "\t \t \t\n", "dump_superblock", "\n"); + assert_main(tctx->dvt_pmem_file, "-f", "file_path"); + assert_int_equal(1, dvt_fake_get_lines_called); + /* commands that modify tree must have '-w' also */ dvt_fake_get_file_exists_result = true; set_fake_inputs("ls", "rm [0]");