From 4ffe15bf6aec1ef8a14d3c070fc6b9bbb40cd4b0 Mon Sep 17 00:00:00 2001 From: Alasdair G Kergon Date: Sat, 30 Jul 2016 02:05:50 +0100 Subject: [PATCH] tools: Unify stripesize parameter validation. Move it all into get_stripe_params(). Some code paths missed --stripesize checks. E.g. lvcreate --type raid4 -i1 --- WHATS_NEW | 1 + lib/metadata/segtype.h | 3 +++ tools/lvconvert.c | 6 +++--- tools/lvcreate.c | 14 ++------------ tools/toollib.c | 40 ++++++++++++++++++++++++---------------- tools/toollib.h | 4 ++-- 6 files changed, 35 insertions(+), 33 deletions(-) diff --git a/WHATS_NEW b/WHATS_NEW index 8474644fd..2e81ab0d0 100644 --- a/WHATS_NEW +++ b/WHATS_NEW @@ -1,5 +1,6 @@ Version 2.02.163 - ================================= + Unify stripe size validation into get_stripe_params to catch missing cases. Further lvconvert validation logic refactoring. Version 2.02.162 - 28th July 2016 diff --git a/lib/metadata/segtype.h b/lib/metadata/segtype.h index 2afb02a8e..f39007f56 100644 --- a/lib/metadata/segtype.h +++ b/lib/metadata/segtype.h @@ -131,6 +131,9 @@ struct dev_manager; #define segtype_is_virtual(segtype) ((segtype)->flags & SEG_VIRTUAL ? 1 : 0) #define segtype_is_unknown(segtype) ((segtype)->flags & SEG_UNKNOWN ? 1 : 0) +#define segtype_supports_stripe_size(segtype) \ + ((segtype_is_striped(segtype) || (segtype_is_raid(segtype) && !segtype_is_raid1(segtype))) ? 1 : 0) + #define seg_is_cache(seg) segtype_is_cache((seg)->segtype) #define seg_is_cache_pool(seg) segtype_is_cache_pool((seg)->segtype) #define seg_is_linear(seg) (seg_is_striped(seg) && ((seg)->area_count == 1)) diff --git a/tools/lvconvert.c b/tools/lvconvert.c index 4a5956ec0..33c7ffb9d 100644 --- a/tools/lvconvert.c +++ b/tools/lvconvert.c @@ -812,11 +812,10 @@ static int _read_params(struct cmd_context *cmd, int argc, char **argv, /* FIXME This is incomplete */ if (_mirror_or_raid_type_requested(cmd, lp->type_str) || _raid0_type_requested(lp->type_str) || _striped_type_requested(lp->type_str) || lp->repair || lp->mirrorlog || lp->corelog) { - if (!get_stripe_params(cmd, &lp->stripes, &lp->stripe_size)) + if (!get_stripe_params(cmd, lp->segtype, &lp->stripes, &lp->stripe_size)) return_0; if (_raid0_type_requested(lp->type_str) || _striped_type_requested(lp->type_str)) - /* FIXME Shouldn't need to override get_stripe_params which defaults to 1 stripe (i.e. linear)! */ /* The default keeps existing number of stripes, handled inside the library code */ if (!arg_is_set(cmd, stripes_long_ARG) && !_linear_type_requested(lp->type_str)) @@ -3145,7 +3144,8 @@ static int _lvconvert_pool(struct cmd_context *cmd, if (!_lvconvert_update_pool_params(pool_lv, lp)) return_0; - if (!get_stripe_params(cmd, &lp->stripes, &lp->stripe_size)) + if (!get_stripe_params(cmd, get_segtype_from_string(cmd, SEG_TYPE_NAME_STRIPED), + &lp->stripes, &lp->stripe_size)) return_0; if (!archive(vg)) diff --git a/tools/lvcreate.c b/tools/lvcreate.c index 5e3ddbb31..253fab897 100644 --- a/tools/lvcreate.c +++ b/tools/lvcreate.c @@ -495,9 +495,7 @@ static int _read_raid_params(struct cmd_context *cmd, lp->segtype->name); return 0; } - - } else if (!lp->stripe_size) - lp->stripe_size = find_config_tree_int(cmd, metadata_stripesize_CFG, NULL) * 2; + } if (arg_is_set(cmd, mirrors_ARG) && segtype_is_raid(lp->segtype) && !segtype_is_raid1(lp->segtype) && !segtype_is_raid10(lp->segtype)) { @@ -572,14 +570,6 @@ static int _read_mirror_and_raid_params(struct cmd_context *cmd, return 0; } - /* - * FIXME This is working around a bug in get_stripe_params() where - * stripes is incorrectly assumed to be 1 when it is not supplied - * leading to the actual value of stripesize getting lost. - */ - if (arg_is_set(cmd, stripesize_ARG)) - lp->stripe_size = arg_uint_value(cmd, stripesize_ARG, 0); - if (!is_power_of_2(lp->region_size)) { log_error("Region size (%" PRIu32 ") must be a power of 2", lp->region_size); @@ -1044,7 +1034,7 @@ static int _lvcreate_params(struct cmd_context *cmd, if (!_lvcreate_name_params(cmd, &argc, &argv, lp) || !_read_size_params(cmd, lp, lcp) || - !get_stripe_params(cmd, &lp->stripes, &lp->stripe_size) || + !get_stripe_params(cmd, lp->segtype, &lp->stripes, &lp->stripe_size) || (lp->create_pool && !get_pool_params(cmd, lp->segtype, &lp->passed_args, &lp->pool_metadata_size, &lp->pool_metadata_spare, diff --git a/tools/toollib.c b/tools/toollib.c index a76599c3e..4cd902097 100644 --- a/tools/toollib.c +++ b/tools/toollib.c @@ -1273,18 +1273,32 @@ int get_pool_params(struct cmd_context *cmd, /* * Generic stripe parameter checks. */ -static int _validate_stripe_params(struct cmd_context *cmd, uint32_t *stripes, - uint32_t *stripe_size) +static int _validate_stripe_params(struct cmd_context *cmd, const struct segment_type *segtype, + uint32_t *stripes, uint32_t *stripe_size) { - if (*stripes == 1 && *stripe_size) { + int stripe_size_required = segtype_supports_stripe_size(segtype); + + if (!stripe_size_required && *stripe_size) { + log_print_unless_silent("Ignoring stripesize argument for %s devices.", segtype->name); + *stripe_size = 0; + } else if (segtype_is_striped(segtype) && *stripes == 1 && *stripe_size) { log_print_unless_silent("Ignoring stripesize argument with single stripe."); + stripe_size_required = 0; *stripe_size = 0; } - if (*stripes > 1 && !*stripe_size) { - *stripe_size = find_config_tree_int(cmd, metadata_stripesize_CFG, NULL) * 2; - log_print_unless_silent("Using default stripesize %s.", - display_size(cmd, (uint64_t) *stripe_size)); + if (stripe_size_required) { + if (!*stripe_size) { + *stripe_size = find_config_tree_int(cmd, metadata_stripesize_CFG, NULL) * 2; + log_print_unless_silent("Using default stripesize %s.", + display_size(cmd, (uint64_t) *stripe_size)); + } + + if (*stripe_size < STRIPE_SIZE_MIN || !is_power_of_2(*stripe_size)) { + log_error("Invalid stripe size %s.", + display_size(cmd, (uint64_t) *stripe_size)); + return 0; + } } if (*stripes < 1 || *stripes > MAX_STRIPES) { @@ -1293,13 +1307,6 @@ static int _validate_stripe_params(struct cmd_context *cmd, uint32_t *stripes, return 0; } - if (*stripes > 1 && (*stripe_size < STRIPE_SIZE_MIN || - !is_power_of_2(*stripe_size))) { - log_error("Invalid stripe size %s.", - display_size(cmd, (uint64_t) *stripe_size)); - return 0; - } - return 1; } @@ -1309,9 +1316,10 @@ static int _validate_stripe_params(struct cmd_context *cmd, uint32_t *stripes, * power of 2, we must divide UINT_MAX by four and add 1 (to round it * up to the power of 2) */ -int get_stripe_params(struct cmd_context *cmd, uint32_t *stripes, uint32_t *stripe_size) +int get_stripe_params(struct cmd_context *cmd, const struct segment_type *segtype, uint32_t *stripes, uint32_t *stripe_size) { /* stripes_long_ARG takes precedence (for lvconvert) */ + /* FIXME Cope with relative +/- changes for lvconvert. */ *stripes = arg_uint_value(cmd, arg_is_set(cmd, stripes_long_ARG) ? stripes_long_ARG : stripes_ARG, 1); *stripe_size = arg_uint_value(cmd, stripesize_ARG, 0); @@ -1328,7 +1336,7 @@ int get_stripe_params(struct cmd_context *cmd, uint32_t *stripes, uint32_t *stri } } - return _validate_stripe_params(cmd, stripes, stripe_size); + return _validate_stripe_params(cmd, segtype, stripes, stripe_size); } static int _validate_cachepool_params(const char *name, diff --git a/tools/toollib.h b/tools/toollib.h index 42e7f7a7d..ed5ec7635 100644 --- a/tools/toollib.h +++ b/tools/toollib.h @@ -201,8 +201,8 @@ int get_pool_params(struct cmd_context *cmd, thin_discards_t *discards, int *zero); -int get_stripe_params(struct cmd_context *cmd, uint32_t *stripes, - uint32_t *stripe_size); +int get_stripe_params(struct cmd_context *cmd, const struct segment_type *segtype, + uint32_t *stripes, uint32_t *stripe_size); int get_cache_params(struct cmd_context *cmd, cache_mode_t *cache_mode,