From 66434e3220daf0fcb6c53ca3865e12c1798061e7 Mon Sep 17 00:00:00 2001 From: Yury Luneff Date: Tue, 20 Oct 2026 10:40:25 +0300 Subject: [PATCH 1/2] Fix vitastor-cli rm-data ignoring offsets with size suffixes rm-data read --min-offset and --max-offset with uint64_value(), which returns 0 for "8M", "-8M" or "0x1000", and 0 means "from the start" and "no upper limit". So a range given with a suffix removed the whole inode instead of a part of it, and the command reported success. Parse offsets as decimal or 0x hex with an optional K/M/G/T suffix, reject signs and values that overflow 64 bits, and fail with EINVAL before removing anything if a value is invalid, the range is empty or reversed, or --max-offset is explicitly 0 (omit it for no limit). --- docs/usage/cli.en.md | 1 + docs/usage/cli.ru.md | 1 + src/cmd/cli.cpp | 1 + src/cmd/cli_rm_data.cpp | 64 +++++++++++++++++++++++++++++++++++++-- tests/test_rm.sh | 67 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 2 deletions(-) diff --git a/docs/usage/cli.en.md b/docs/usage/cli.en.md index 0b700725..11c8646d 100644 --- a/docs/usage/cli.en.md +++ b/docs/usage/cli.en.md @@ -252,6 +252,7 @@ Remove inode data without changing metadata. Requires more memory, but allows to show correct removal progress. --min-offset Purge only data starting with specified offset. --max-offset Purge only data before specified offset. + Offsets are in bytes or with a K/M/G/T unit suffix, decimal or 0x hex. --client_wait_up_timeout 16 Timeout for waiting until PGs are up in seconds. ``` diff --git a/docs/usage/cli.ru.md b/docs/usage/cli.ru.md index e4787d15..d6ea8bb9 100644 --- a/docs/usage/cli.ru.md +++ b/docs/usage/cli.ru.md @@ -261,6 +261,7 @@ vitastor-cli dd [iimg= | if=] [oimg= | of=] [bs=1M] \ Требует больше памяти, но позволяет правильно печатать прогресс удаления. --min-offset Удалять только данные, начиная с заданного смещения. --max-offset Удалять только данные до (исключительно) заданного смещения. + Смещения задаются в байтах или с суффиксом K/M/G/T, в десятичном виде или в hex с 0x. --client_wait_up_timeout 16 Время ожидания поднятия PG в секундах. ``` diff --git a/src/cmd/cli.cpp b/src/cmd/cli.cpp index 28ba05af..d9e51521 100644 --- a/src/cmd/cli.cpp +++ b/src/cmd/cli.cpp @@ -122,6 +122,7 @@ static const char* help_text = " Requires more memory, but allows to show correct removal progress.\n" " --min-offset Purge only data starting with specified offset.\n" " --max-offset Purge only data before specified offset.\n" + " Offsets are in bytes or with a K/M/G/T unit suffix, decimal or 0x hex.\n" " --client_wait_up_timeout 16 Timeout for waiting until PGs are up in seconds.\n" "\n" "vitastor-cli merge-data [--target ]\n" diff --git a/src/cmd/cli_rm_data.cpp b/src/cmd/cli_rm_data.cpp index c2510848..4c555f94 100644 --- a/src/cmd/cli_rm_data.cpp +++ b/src/cmd/cli_rm_data.cpp @@ -1,6 +1,7 @@ // Copyright (c) Vitaliy Filippov, 2019+ // License: VNPL-1.1 (see README.md for details) +#include #include "cli.h" #include "cluster_client.h" @@ -317,8 +318,67 @@ struct rm_inode_t } }; +// Decimal or 0x hex, without sign, octal or overflow +static bool parse_rm_number(std::string s, uint64_t & value) +{ + int base = 10; + if (s.size() > 2 && s[0] == '0' && (s[1] == 'x' || s[1] == 'X')) + { + s = s.substr(2); + base = 16; + } + if (s == "" || s.find_first_not_of(base == 16 ? "0123456789abcdefABCDEF" : "0123456789") != std::string::npos) + return false; + errno = 0; + value = strtoull(s.c_str(), NULL, base); + return !errno; +} + +// Command-line values are strings with optional K/M/G/T suffixes, internal callers pass numbers +static bool parse_rm_offset(const json11::Json & value, uint64_t & offset) +{ + if (!value.is_string()) + { + offset = value.uint64_value(); + return true; + } + std::string s = value.string_value(); + int shift = 0; + switch (s.size() ? tolower(s.back()) : 0) + { + case 'k': shift = 10; break; + case 'm': shift = 20; break; + case 'g': shift = 30; break; + case 't': shift = 40; break; + } + if (shift) + s.pop_back(); + if (!parse_rm_number(s, offset) || offset > (UINT64_MAX >> shift)) + return false; + offset <<= shift; + return true; +} + std::function cli_tool_t::start_rm_data(json11::Json cfg) { + uint64_t min_offset = 0, max_offset = 0; + std::string error; + if (!parse_rm_offset(cfg["min_offset"], min_offset)) + error = "Invalid --min-offset value: "+cfg["min_offset"].as_string(); + else if (!parse_rm_offset(cfg["max_offset"], max_offset)) + error = "Invalid --max-offset value: "+cfg["max_offset"].as_string(); + else if (!max_offset && cfg["max_offset"].is_string()) + error = "--max-offset 0 is invalid, omit it to remove data up to the end"; + else if (max_offset && min_offset >= max_offset) + error = "--min-offset must be less than --max-offset"; + if (error != "") + { + return [error](cli_result_t & result) + { + result = (cli_result_t){ .err = EINVAL, .text = error }; + return true; + }; + } auto remover = new rm_inode_t(); remover->parent = this; remover->inode = cfg["inode"].uint64_value(); @@ -329,8 +389,8 @@ std::function cli_tool_t::start_rm_data(json11::Json cfg) } remover->down_ok = cfg["down_ok"].bool_value(); remover->pool_id = INODE_POOL(remover->inode); - remover->min_offset = cfg["min_offset"].uint64_value(); - remover->max_offset = cfg["max_offset"].uint64_value(); + remover->min_offset = min_offset; + remover->max_offset = max_offset; return [remover](cli_result_t & result) { remover->loop(); diff --git a/tests/test_rm.sh b/tests/test_rm.sh index 51d6d57e..98067aab 100755 --- a/tests/test_rm.sh +++ b/tests/test_rm.sh @@ -24,4 +24,71 @@ sz=$(stat -c %s ./testdata/bin/testimg.bin) [[ "$sz" = 67108864 ]] || format_error "File size is $sz bytes after resize to 64 M" +# test rm-data offset parsing and validation + +$VITASTOR_CLI create -s 32M rmimg +dd if=/dev/urandom of=./testdata/bin/rmimg.bin bs=1M count=32 +$VITASTOR_CLI dd iodepth=4 bs=1M if=./testdata/bin/rmimg.bin oimg=rmimg + +check_rmimg() +{ + rm -f ./testdata/bin/check.bin + $VITASTOR_CLI dd iodepth=4 bs=1M conv=nosparse iimg=rmimg of=./testdata/bin/check.bin + cmp ./testdata/bin/check.bin ./testdata/bin/rmimg.bin || format_error "$1" +} + +zero_rmimg() +{ + dd if=/dev/zero of=./testdata/bin/rmimg.bin bs=1M seek=$1 count=$2 conv=notrunc +} + +check_rmimg "Image data differs after write" + +RM_DATA="$VITASTOR_CLI rm-data --pool 1 --inode $($VITASTOR_CLI ls --json rmimg | jq -r '.[0].inode_num')" + +# Bad offsets and empty ranges must fail without removing anything +while IFS='|' read -r -u 3 args err; do + if out=$($RM_DATA $args 2>&1); then + format_error "rm-data $args should fail" + fi + [[ "$out" = "$err" ]] || format_error "rm-data $args failed with an unexpected error: $out" + check_rmimg "rm-data $args removed data" +done 3< Date: Tue, 20 Oct 2026 10:42:18 +0300 Subject: [PATCH 2/2] Accept pool names and hex inode numbers in vitastor-cli rm-data rm-data read --pool and --inode with uint64_value(), so a pool name became 0 and the pool was silently taken from the inode's pool bits, i.e. "--pool --inode " removed data in a different pool than requested. A hex --inode became 0 and rm-data "removed" inode 0, reporting success. Resolve pool names like ls does (ENOENT for an unknown pool), parse --inode as decimal or 0x hex, and fail with EINVAL on anything else before removing data. --- docs/usage/cli.en.md | 4 +-- docs/usage/cli.ru.md | 4 +-- src/cmd/cli.cpp | 4 +-- src/cmd/cli_rm_data.cpp | 54 ++++++++++++++++++++++++++++++++---- tests/test_rm.sh | 61 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 11 deletions(-) diff --git a/docs/usage/cli.en.md b/docs/usage/cli.en.md index 11c8646d..987a3486 100644 --- a/docs/usage/cli.en.md +++ b/docs/usage/cli.en.md @@ -243,9 +243,9 @@ Flatten a layer, i.e. merge data and detach it from parents. ## rm-data -`vitastor-cli rm-data --pool --inode [--wait-list] [--min-offset ]` +`vitastor-cli rm-data --pool --inode [--wait-list] [--min-offset ]` -Remove inode data without changing metadata. +Remove inode data without changing metadata. `` may be decimal or 0x hex. ``` --wait-list Retrieve full objects listings before starting to remove objects. diff --git a/docs/usage/cli.ru.md b/docs/usage/cli.ru.md index d6ea8bb9..3b88b71e 100644 --- a/docs/usage/cli.ru.md +++ b/docs/usage/cli.ru.md @@ -252,9 +252,9 @@ vitastor-cli dd [iimg= | if=] [oimg= | of=] [bs=1M] \ ## rm-data -`vitastor-cli rm-data --pool --inode [--wait-list] [--min-offset ]` +`vitastor-cli rm-data --pool --inode [--wait-list] [--min-offset ]` -Удалить данные инода, не меняя метаданные образов. +Удалить данные инода, не меняя метаданные образов. `` можно задать в десятичном виде или в hex с 0x. ``` --wait-list Сначала запросить полный листинг объектов, а потом начать удалять. diff --git a/src/cmd/cli.cpp b/src/cmd/cli.cpp index d9e51521..9ed352e4 100644 --- a/src/cmd/cli.cpp +++ b/src/cmd/cli.cpp @@ -116,8 +116,8 @@ static const char* help_text = "vitastor-cli flatten \n" " Flatten a layer, i.e. merge data and detach it from parents.\n" "\n" - "vitastor-cli rm-data --pool --inode [--wait-list] [--min-offset ]\n" - " Remove inode data without changing metadata.\n" + "vitastor-cli rm-data --pool --inode [--wait-list] [--min-offset ]\n" + " Remove inode data without changing metadata. may be decimal or 0x hex.\n" " --wait-list Retrieve full objects listings before starting to remove objects.\n" " Requires more memory, but allows to show correct removal progress.\n" " --min-offset Purge only data starting with specified offset.\n" diff --git a/src/cmd/cli_rm_data.cpp b/src/cmd/cli_rm_data.cpp index 4c555f94..94fa05b6 100644 --- a/src/cmd/cli_rm_data.cpp +++ b/src/cmd/cli_rm_data.cpp @@ -24,6 +24,7 @@ struct rm_inode_t { uint64_t inode = 0; pool_id_t pool_id = 0; + std::string pool_name; uint64_t min_offset = 0; uint64_t max_offset = 0; bool down_ok = false; @@ -303,6 +304,25 @@ struct rm_inode_t goto resume_1; if (state == 100) return; + if (pool_name != "") + { + pool_id = 0; + for (auto & pp: parent->cli->st_cli->pool_config) + { + if (pp.second.name == pool_name) + { + pool_id = pp.first; + break; + } + } + if (!pool_id) + { + result = (cli_result_t){ .err = ENOENT, .text = "Pool "+pool_name+" does not exist" }; + state = 100; + return; + } + inode = INODE_WITH_POOL(pool_id, inode); + } if (!pool_id) { result = (cli_result_t){ .err = EINVAL, .text = "Pool is not specified" }; @@ -359,11 +379,34 @@ static bool parse_rm_offset(const json11::Json & value, uint64_t & offset) return true; } +static bool parse_rm_inode(const json11::Json & value, uint64_t & inode) +{ + if (!value.is_string()) + { + inode = value.uint64_value(); + return true; + } + return parse_rm_number(value.string_value(), inode); +} + +// A non-numeric string is a pool name, resolved after loading the pool configuration +static bool parse_rm_pool(const json11::Json & value, uint64_t & pool_id, std::string & pool_name) +{ + pool_id = value.uint64_value(); + if (value.is_string() && !pool_id && value.string_value() != "0") + pool_name = value.string_value(); + return pool_id < POOL_ID_MAX && (!value.is_string() || value.string_value() != ""); +} + std::function cli_tool_t::start_rm_data(json11::Json cfg) { - uint64_t min_offset = 0, max_offset = 0; - std::string error; - if (!parse_rm_offset(cfg["min_offset"], min_offset)) + uint64_t inode = 0, pool_id = 0, min_offset = 0, max_offset = 0; + std::string pool_name, error; + if (!parse_rm_inode(cfg["inode"], inode)) + error = "Invalid --inode value: "+cfg["inode"].as_string(); + else if (!parse_rm_pool(cfg["pool"], pool_id, pool_name)) + error = "Invalid --pool value: "+cfg["pool"].as_string(); + else if (!parse_rm_offset(cfg["min_offset"], min_offset)) error = "Invalid --min-offset value: "+cfg["min_offset"].as_string(); else if (!parse_rm_offset(cfg["max_offset"], max_offset)) error = "Invalid --max-offset value: "+cfg["max_offset"].as_string(); @@ -381,8 +424,9 @@ std::function cli_tool_t::start_rm_data(json11::Json cfg) } auto remover = new rm_inode_t(); remover->parent = this; - remover->inode = cfg["inode"].uint64_value(); - remover->pool_id = cfg["pool"].uint64_value(); + remover->inode = inode; + remover->pool_id = pool_id; + remover->pool_name = pool_name; if (remover->pool_id) { remover->inode = (remover->inode & (((uint64_t)1 << (64-POOL_ID_BITS)) - 1)) | (((uint64_t)remover->pool_id) << (64-POOL_ID_BITS)); diff --git a/tests/test_rm.sh b/tests/test_rm.sh index 98067aab..01573e86 100755 --- a/tests/test_rm.sh +++ b/tests/test_rm.sh @@ -91,4 +91,65 @@ $RM_DATA --min-offset 30M zero_rmimg 30 2 check_rmimg "rm-data --min-offset 30M removed wrong data" +# test rm-data pool names and hex inode numbers + +$VITASTOR_CLI create-pool testpool2 -s 2 -n 4 --failure_domain osd +wait_pool_up 60 2 2 4 + +$VITASTOR_CLI create -s 32M -p testpool2 rmimg2 +dd if=/dev/urandom of=./testdata/bin/rmimg2.bin bs=1M count=32 +$VITASTOR_CLI dd iodepth=4 bs=1M if=./testdata/bin/rmimg2.bin oimg=rmimg2 + +zero_rmimg2() +{ + dd if=/dev/zero of=./testdata/bin/rmimg2.bin bs=1M seek=$1 count=$2 conv=notrunc +} + +check_rmimg2() +{ + check_rmimg "$1" + rm -f ./testdata/bin/check.bin + $VITASTOR_CLI dd iodepth=4 bs=1M conv=nosparse iimg=rmimg2 of=./testdata/bin/check.bin + cmp ./testdata/bin/check.bin ./testdata/bin/rmimg2.bin || format_error "$1" +} + +check_rmimg2 "Image data differs after write" + +INODE1=$($VITASTOR_CLI ls --json rmimg | jq -r '.[0].inode_num') +INODE2=$($VITASTOR_CLI ls --json rmimg2 | jq -r '.[0].inode_num') +[[ "$($VITASTOR_CLI ls --json rmimg2 | jq -r '.[0].pool_id')" = 2 ]] || format_error "rmimg2 is not in pool 2" + +# Bad inode numbers and unknown pools must fail without removing anything +while IFS='|' read -r -u 3 args err; do + if out=$($VITASTOR_CLI rm-data $args 2>&1); then + format_error "rm-data $args should fail" + fi + [[ "$out" = "$err" ]] || format_error "rm-data $args failed with an unexpected error: $out" + check_rmimg2 "rm-data $args removed data" +done 3<