From 4d2311655b3e5d1d93f69c3d15253d046cfd8d70 Mon Sep 17 00:00:00 2001 From: Zdenek Kabelac Date: Sun, 16 Jul 2023 17:35:20 +0200 Subject: [PATCH] lvm-exec: refactor code Add prepare_exec_args() for reading option list for thin/cache_repair, thin/cache_check. --- lib/activate/dev_manager.c | 28 ++---------- lib/config/defaults.h | 2 + lib/metadata/thin_manip.c | 25 ++--------- lib/misc/lvm-exec.c | 31 +++++++++++++ lib/misc/lvm-exec.h | 4 ++ tools/lvconvert.c | 89 +++++++++++--------------------------- 6 files changed, 69 insertions(+), 110 deletions(-) diff --git a/lib/activate/dev_manager.c b/lib/activate/dev_manager.c index 941604fb1..1ab63d41c 100644 --- a/lib/activate/dev_manager.c +++ b/lib/activate/dev_manager.c @@ -2457,8 +2457,6 @@ static int _pool_callback(struct dm_tree_node *node, dm_node_callback_t type, void *cb_data) { int ret, status = 0, fd; - const struct dm_config_node *cn; - const struct dm_config_value *cv; const struct pool_cb_data *data = cb_data; const struct logical_volume *pool_lv = data->pool_lv; const struct logical_volume *mlv = first_seg(pool_lv)->metadata_lv; @@ -2466,11 +2464,11 @@ static int _pool_callback(struct dm_tree_node *node, long buf[64 / sizeof(long)]; /* buffer for short disk header (64B) */ int args = 0; char *mpath; - const char *argv[19] = { /* Max supported 15 args */ + const char *argv[DEFAULT_MAX_EXEC_ARGS + 7] = { /* Max supported 15 args */ find_config_tree_str_allow_empty(cmd, data->exec, NULL) }; - if (!*argv[0]) /* *_check tool is unconfigured/disabled with "" setting */ + if (!argv[0] || !*argv[0]) /* *_check tool is unconfigured/disabled with "" setting */ return 1; if (lv_is_cache_vol(pool_lv)) { @@ -2517,26 +2515,8 @@ static int _pool_callback(struct dm_tree_node *node, } } - if (!(cn = find_config_tree_array(cmd, data->opts, NULL))) { - log_error(INTERNAL_ERROR "Unable to find configuration for pool check options."); - return 0; - } - - for (cv = cn->v; cv && args < 16; cv = cv->next) { - if (cv->type != DM_CFG_STRING) { - log_error("Invalid string in config file: " - "global/%s_check_options.", - data->global); - return 0; - } - if (cv->v.str[0]) - argv[++args] = cv->v.str; - } - - if (args == 16) { - log_error("Too many options for %s command.", argv[0]); - return 0; - } + if (!prepare_exec_args(cmd, argv, &args, data->opts)) + return_0; argv[++args] = mpath; diff --git a/lib/config/defaults.h b/lib/config/defaults.h index dd40ff3bf..2a97baded 100644 --- a/lib/config/defaults.h +++ b/lib/config/defaults.h @@ -105,6 +105,8 @@ # define DEFAULT_DMEVENTD_PATH DMEVENTD_PATH #endif +#define DEFAULT_MAX_EXEC_ARGS 15 /* Max number of accepted options args */ + #ifdef THIN_CHECK_NEEDS_CHECK # define DEFAULT_THIN_CHECK_OPTION1 "-q" # define DEFAULT_THIN_CHECK_OPTION2 "--clear-needs-check-flag" diff --git a/lib/metadata/thin_manip.c b/lib/metadata/thin_manip.c index 492a7dbd2..bb406a3d0 100644 --- a/lib/metadata/thin_manip.c +++ b/lib/metadata/thin_manip.c @@ -460,9 +460,7 @@ int thin_pool_prepare_metadata(struct logical_volume *metadata_lv, { struct cmd_context *cmd = metadata_lv->vg->cmd; char lv_path[PATH_MAX], md_path[64], buffer[512]; - const struct dm_config_node *cn; - const struct dm_config_value *cv; - const char *argv[20] = { /* Max supported 15 option args */ + const char *argv[DEFAULT_MAX_EXEC_ARGS + 7] = { find_config_tree_str_allow_empty(cmd, global_thin_restore_executable_CFG, NULL) }; int args = 0; @@ -477,25 +475,8 @@ int thin_pool_prepare_metadata(struct logical_volume *metadata_lv, return 0; } - if (!(cn = find_config_tree_array(cmd, global_thin_restore_options_CFG, NULL))) { - log_error(INTERNAL_ERROR "Unable to find configuration for pool check options."); - return 0; - } - - for (cv = cn->v; cv && args < 16; cv = cv->next) { - if (cv->type != DM_CFG_STRING) { - log_error("Invalid string in config file: " - "global/thin_restore_options."); - return 0; - } - if (cv->v.str[0]) - argv[++args] = cv->v.str; - } - - if (args == 16) { - log_error("Too many options for %s command.", argv[0]); - return 0; - } + if (!prepare_exec_args(cmd, argv, &args, global_thin_restore_options_CFG)) + return_0; if (test_mode()) { log_verbose("Test mode: Skipping creation of provisioned thin pool metadata."); diff --git a/lib/misc/lvm-exec.c b/lib/misc/lvm-exec.c index 77a2e0653..172c0fa4a 100644 --- a/lib/misc/lvm-exec.c +++ b/lib/misc/lvm-exec.c @@ -217,3 +217,34 @@ int pipe_close(struct pipe_data *pdata) return (status == 0) ? 1 : 0; } + +int prepare_exec_args(struct cmd_context *cmd, + const char *argv[], int *argc, int options_id) +{ + const struct dm_config_value *cv; + const struct dm_config_node *cn; + + if (!(cn = find_config_tree_array(cmd, options_id, NULL))) { + log_error(INTERNAL_ERROR "Unable to find configuration for %s options.", + argv[0]); + return 0; + } + + for (cv = cn->v; cv; cv = cv->next) { + if (*argc >= DEFAULT_MAX_EXEC_ARGS) { + log_error("Too many options for %s command.", argv[0]); + return 0; + } + + if (cv->type != DM_CFG_STRING) { + log_error("Invalid string in config file: " + "global/%s_options.", argv[0]); + return 0; + } + + if (cv->v.str[0]) + argv[++(*argc)] = cv->v.str; + } + + return 1; +} diff --git a/lib/misc/lvm-exec.h b/lib/misc/lvm-exec.h index 8db9a354e..04291853f 100644 --- a/lib/misc/lvm-exec.h +++ b/lib/misc/lvm-exec.h @@ -67,4 +67,8 @@ FILE *pipe_open(struct cmd_context *cmd, const char *const argv[], int pipe_close(struct pipe_data *pdata); +/* Prepare argv options list */ +int prepare_exec_args(struct cmd_context *cmd, + const char *argv[], int *argc, int options_id); + #endif diff --git a/tools/lvconvert.c b/tools/lvconvert.c index fb8691b31..b3799b5d7 100644 --- a/tools/lvconvert.c +++ b/tools/lvconvert.c @@ -19,8 +19,6 @@ #include "lib/metadata/metadata.h" #include "lvconvert_poll.h" -#define MAX_PDATA_ARGS 10 /* Max number of accepted args for d-m-p-d tools */ - typedef enum { /* Split: * For a mirrored or raid LV, split mirror into two mirrors, optionally tracking @@ -2341,13 +2339,11 @@ static int _lvconvert_thin_pool_repair(struct cmd_context *cmd, const char *dmdir = dm_dir(); const char *thin_dump = find_config_tree_str_allow_empty(cmd, global_thin_dump_executable_CFG, NULL); - const char *thin_repair = - find_config_tree_str_allow_empty(cmd, global_thin_repair_executable_CFG, NULL); - const struct dm_config_node *cn; - const struct dm_config_value *cv; int ret = 0, status; int args = 0; - const char *argv[MAX_PDATA_ARGS + 7]; /* Max supported args */ + const char *argv[DEFAULT_MAX_EXEC_ARGS + 7] = { /* Max supported args */ + find_config_tree_str_allow_empty(cmd, global_thin_repair_executable_CFG, NULL) + }; char *dm_name, *trans_id_str; char meta_path[PATH_MAX]; char pms_path[PATH_MAX]; @@ -2357,9 +2353,15 @@ static int _lvconvert_thin_pool_repair(struct cmd_context *cmd, struct pipe_data pdata; FILE *f; - if (!thin_repair || !thin_repair[0]) { - log_error("Thin repair commnand is not configured. Repair is disabled."); - return 0; /* Checking disabled */ + if (!argv[0] || !*argv[0]) { + log_error("Thin repair command is not configured. Repair is disabled."); + return 0; + } + + if (thin_pool_is_active(pool_lv)) { + log_error("Cannot repair active pool %s. Use lvchange -an first.", + display_lvname(pool_lv)); + return 0; } pmslv = pool_lv->vg->pool_metadata_spare_lv; @@ -2388,36 +2390,13 @@ static int _lvconvert_thin_pool_repair(struct cmd_context *cmd, return 0; } - if (!(cn = find_config_tree_array(cmd, global_thin_repair_options_CFG, NULL))) { - log_error(INTERNAL_ERROR "Unable to find configuration for global/thin_repair_options"); - return 0; - } + if (!prepare_exec_args(cmd, argv, &args, global_thin_repair_options_CFG)) + return_0; - for (cv = cn->v; cv && args < MAX_PDATA_ARGS; cv = cv->next) { - if (cv->type != DM_CFG_STRING) { - log_error("Invalid string in config file: " - "global/thin_repair_options"); - return 0; - } - argv[++args] = cv->v.str; - } - - if (args >= MAX_PDATA_ARGS) { - log_error("Too many options for thin repair command."); - return 0; - } - - argv[0] = thin_repair; argv[++args] = "-i"; argv[++args] = meta_path; argv[++args] = "-o"; argv[++args] = pms_path; - argv[++args] = NULL; - - if (thin_pool_is_active(pool_lv)) { - log_error("Active pools cannot be repaired. Use lvchange -an first."); - return 0; - } if (!activate_lv(cmd, pmslv)) { log_error("Cannot activate pool metadata spare volume %s.", @@ -2439,7 +2418,7 @@ static int _lvconvert_thin_pool_repair(struct cmd_context *cmd, } /* Check matching transactionId when thin-pool is used by lvm2 (transactionId != 0) */ - if (first_seg(pool_lv)->transaction_id && thin_dump[0]) { + if (first_seg(pool_lv)->transaction_id && thin_dump && thin_dump[0]) { argv[0] = thin_dump; argv[1] = pms_path; argv[2] = NULL; @@ -2455,13 +2434,15 @@ static int _lvconvert_thin_pool_repair(struct cmd_context *cmd, (trans_id_str = strstr(meta_path, "transaction=\"")) && (sscanf(trans_id_str + 13, FMTu64, &trans_id) == 1) && (trans_id != first_seg(pool_lv)->transaction_id) && - ((trans_id - 1) != first_seg(pool_lv)->transaction_id)) + ((trans_id - 1) != first_seg(pool_lv)->transaction_id)) { log_error("Transaction id " FMTu64 " from pool \"%s/%s\" " "does not match repaired transaction id " FMTu64 " from %s.", first_seg(pool_lv)->transaction_id, pool_lv->vg->name, pool_lv->name, trans_id, pms_path); + ret = 0; + } (void) pipe_close(&pdata); /* killing pipe */ } @@ -2539,13 +2520,11 @@ static int _lvconvert_cache_repair(struct cmd_context *cmd, struct dm_list *pvh, int poolmetadataspare) { const char *dmdir = dm_dir(); - const char *cache_repair = - find_config_tree_str_allow_empty(cmd, global_cache_repair_executable_CFG, NULL); - const struct dm_config_node *cn; - const struct dm_config_value *cv; int ret = 0, status; int args = 0; - const char *argv[MAX_PDATA_ARGS + 7]; /* Max supported args */ + const char *argv[DEFAULT_MAX_EXEC_ARGS + 7] = { /* Max supported args */ + find_config_tree_str_allow_empty(cmd, global_cache_repair_executable_CFG, NULL) + }; char *dm_name; char meta_path[PATH_MAX]; char pms_path[PATH_MAX]; @@ -2561,8 +2540,8 @@ static int _lvconvert_cache_repair(struct cmd_context *cmd, pool_lv = lv_is_cache_pool(cache_lv) ? cache_lv : first_seg(cache_lv)->pool_lv; mlv = first_seg(pool_lv)->metadata_lv; - if (!cache_repair || !cache_repair[0]) { - log_error("Cache repair commnand is not configured. Repair is disabled."); + if (!argv[0] || !*argv[0]) { + log_error("Cache repair command is not configured. Repair is disabled."); return 0; /* Checking disabled */ } @@ -2592,31 +2571,13 @@ static int _lvconvert_cache_repair(struct cmd_context *cmd, return 0; } - if (!(cn = find_config_tree_array(cmd, global_cache_repair_options_CFG, NULL))) { - log_error(INTERNAL_ERROR "Unable to find configuration for global/cache_repair_options"); - return 0; - } + if (!prepare_exec_args(cmd, argv, &args, global_cache_repair_options_CFG)) + return_0; - for (cv = cn->v; cv && args < MAX_PDATA_ARGS; cv = cv->next) { - if (cv->type != DM_CFG_STRING) { - log_error("Invalid string in config file: " - "global/cache_repair_options"); - return 0; - } - argv[++args] = cv->v.str; - } - - if (args >= MAX_PDATA_ARGS) { - log_error("Too many options for cache repair command."); - return 0; - } - - argv[0] = cache_repair; argv[++args] = "-i"; argv[++args] = meta_path; argv[++args] = "-o"; argv[++args] = pms_path; - argv[++args] = NULL; if (lv_is_active(cache_lv)) { log_error("Only inactive cache can be repaired.");