From 7e45891257db2f6510a240159657cdd9d1bb24aa Mon Sep 17 00:00:00 2001 From: TapTap Date: Sat, 12 Sep 2026 14:54:47 +0200 Subject: [PATCH] refactor(cli): dedupe server/client option parsing and tighten CLI tests - server_cli: handle --password-file/--early-input/--iconv via arg_has_value in one place, removing the unreachable duplicate separate-form arms while keeping both --opt VALUE and --opt=VALUE working - client_cli: factor the triplicated --delta-block/--block-size range check into set_delta_block_size(); drop the redundant use_metadata assignment after identity_parse_copy_as (the parser already forces it) - tests: cover both spellings of --iconv/--delta-block, make the archive short-form test actually call parse_args, add delta-block invalid cases --- src/client/client_cli.c | 38 +++++++++++----------- src/server/server_cli.c | 70 +++++++++++++++-------------------------- tests/test_client_cli.c | 35 +++++++++++++++------ tests/test_server_cli.c | 19 +++++++---- 4 files changed, 83 insertions(+), 79 deletions(-) diff --git a/src/client/client_cli.c b/src/client/client_cli.c index b2af76f..44856de 100644 --- a/src/client/client_cli.c +++ b/src/client/client_cli.c @@ -389,6 +389,21 @@ static int parse_ull_arg(const char* val, unsigned long long* out, const char* o return 0; } +/* Apply a --delta-block/--block-size value (both spellings and both the inline + * and separate argument forms share this one range check). An out-of-range + * value warns once and leaves the configured default untouched. Returns 0 on + * success, -1 on a non-numeric value. */ +static int set_delta_block_size(Config* config, const char* value) { + unsigned long long val; + if (parse_ull_arg(value, &val, "--block-size/--delta-block") != 0) + return -1; + if (val >= DELTA_BLOCK_SIZE_MIN && val <= DELTA_BLOCK_SIZE_MAX) + config->delta_block_size = (uint32_t)val; + else + log_message(LOG_LEVEL_WARNING, "block size value %llu out of range, using default", val); + return 0; +} + /* Parse a byte count with an optional single-letter binary suffix (K/M/G/T/P/E). * When allow_zero is false, a bare 0 is rejected (size limits use true, since 0 * means "no limit"). Returns 0 on success, -1 on error. */ @@ -1094,33 +1109,18 @@ int parse_args(Config* config, int argc, char* argv[], int* positional_args, "--include") != 0) return -1; } else if (strncmp(argv[i], "--delta-block=", 14) == 0) { - unsigned long long val; - if (parse_ull_arg(argv[i] + 14, &val, "--block-size/--delta-block") != 0) + if (set_delta_block_size(config, argv[i] + 14) != 0) return -1; - if (val >= DELTA_BLOCK_SIZE_MIN && val <= DELTA_BLOCK_SIZE_MAX) - config->delta_block_size = (uint32_t)val; - else - log_message(LOG_LEVEL_WARNING, "block size value %llu out of range, using default", val); } else if (strncmp(argv[i], "--block-size=", 13) == 0) { - unsigned long long val; - if (parse_ull_arg(argv[i] + 13, &val, "--block-size/--delta-block") != 0) + if (set_delta_block_size(config, argv[i] + 13) != 0) return -1; - if (val >= DELTA_BLOCK_SIZE_MIN && val <= DELTA_BLOCK_SIZE_MAX) - config->delta_block_size = (uint32_t)val; - else - log_message(LOG_LEVEL_WARNING, "block size value %llu out of range, using default", val); } else if (opt_is(argv[i], "--delta-block", "--block-size")) { if (i + 1 >= argc) { log_message(LOG_LEVEL_ERROR, "missing argument for %s", argv[i]); return -1; } - unsigned long long val; - if (parse_ull_arg(argv[++i], &val, "--block-size/--delta-block") != 0) + if (set_delta_block_size(config, argv[++i]) != 0) return -1; - if (val >= DELTA_BLOCK_SIZE_MIN && val <= DELTA_BLOCK_SIZE_MAX) - config->delta_block_size = (uint32_t)val; - else - log_message(LOG_LEVEL_WARNING, "block size value %llu out of range, using default", val); } else if (opt_is(argv[i], "--delta-max", NULL)) { if (i + 1 >= argc) { log_message(LOG_LEVEL_ERROR, "missing argument for %s", argv[i]); @@ -1431,7 +1431,6 @@ int parse_args(Config* config, int argc, char* argv[], int* positional_args, } else if (strncmp(argv[i], "--copy-as=", 10) == 0) { if (identity_parse_copy_as(config, argv[i] + 10) != 0) return -1; - config->use_metadata = true; } else if (opt_is(argv[i], "--copy-as", NULL)) { if (i + 1 >= argc) { log_message(LOG_LEVEL_ERROR, "missing argument for %s", argv[i]); @@ -1439,7 +1438,6 @@ int parse_args(Config* config, int argc, char* argv[], int* positional_args, } if (identity_parse_copy_as(config, argv[++i]) != 0) return -1; - config->use_metadata = true; } else if (strncmp(argv[i], "--outbuf=", 9) == 0) { if (set_outbuf_option(config, argv[i] + 9) != 0) return -1; diff --git a/src/server/server_cli.c b/src/server/server_cli.c index 120e7bd..6ea42d8 100644 --- a/src/server/server_cli.c +++ b/src/server/server_cli.c @@ -64,6 +64,7 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, server_cli_options_default(opts); for (int i = 1; i < argc; i++) { + const char* inline_value = NULL; if (arg_is(argv[i], "--help")) { opts->show_help = true; return 1; @@ -108,18 +109,24 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, } opts->destination_root = argv[++i]; opts->destination_root_set = true; - } else if (arg_is(argv[i], "--password-file")) { - if (i + 1 >= argc) { - set_error(err, err_size, "missing argument for --password-file"); - return -1; + } else if (arg_has_value(argv[i], "--password-file", &inline_value)) { + if (!inline_value) { + if (i + 1 >= argc) { + set_error(err, err_size, "missing argument for --password-file"); + return -1; + } + inline_value = argv[++i]; } - opts->password_file = argv[++i]; - } else if (arg_is(argv[i], "--early-input")) { - if (i + 1 >= argc) { - set_error(err, err_size, "missing argument for --early-input"); - return -1; + opts->password_file = inline_value; + } else if (arg_has_value(argv[i], "--early-input", &inline_value)) { + if (!inline_value) { + if (i + 1 >= argc) { + set_error(err, err_size, "missing argument for --early-input"); + return -1; + } + inline_value = argv[++i]; } - opts->early_input_file = argv[++i]; + opts->early_input_file = inline_value; } else if (arg_is(argv[i], "--address")) { if (i + 1 >= argc) { set_error(err, err_size, "missing argument for --address"); @@ -146,12 +153,15 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, opts->no_super = true; } else if (arg_is(argv[i], "--allow-unauthenticated")) { opts->allow_unauthenticated = true; - } else if (arg_is(argv[i], "--iconv")) { - if (i + 1 >= argc) { - set_error(err, err_size, "missing argument for --iconv"); - return -1; + } else if (arg_has_value(argv[i], "--iconv", &inline_value)) { + if (!inline_value) { + if (i + 1 >= argc) { + set_error(err, err_size, "missing argument for --iconv"); + return -1; + } + inline_value = argv[++i]; } - opts->iconv_spec = argv[++i]; + opts->iconv_spec = inline_value; } else if (arg_is(argv[i], "-p")) { if (i + 1 >= argc) { set_error(err, err_size, "missing argument for -p"); @@ -161,7 +171,6 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, if (parse_port_arg(argv[++i], &opts->port, err, err_size) != 0) return -1; } else { - const char* inline_value = NULL; if (arg_has_value(argv[i], "--config", &inline_value)) { if (!inline_value) { if (i + 1 >= argc) { @@ -171,33 +180,6 @@ int server_cli_parse(int argc, char* argv[], ServerCliOptions* opts, char* err, inline_value = argv[++i]; } opts->config_path = inline_value; - } else if (arg_has_value(argv[i], "--password-file", &inline_value)) { - if (!inline_value) { - if (i + 1 >= argc) { - set_error(err, err_size, "missing argument for --password-file"); - return -1; - } - inline_value = argv[++i]; - } - opts->password_file = inline_value; - } else if (arg_has_value(argv[i], "--early-input", &inline_value)) { - if (!inline_value) { - if (i + 1 >= argc) { - set_error(err, err_size, "missing argument for --early-input"); - return -1; - } - inline_value = argv[++i]; - } - opts->early_input_file = inline_value; - } else if (arg_has_value(argv[i], "--iconv", &inline_value)) { - if (!inline_value) { - if (i + 1 >= argc) { - set_error(err, err_size, "missing argument for --iconv"); - return -1; - } - inline_value = argv[++i]; - } - opts->iconv_spec = inline_value; } else if (arg_has_value(argv[i], "--dparam", &inline_value)) { if (!inline_value) { if (i + 1 >= argc) { @@ -260,4 +242,4 @@ void server_cli_options_free(ServerCliOptions* opts) { free((char**)opts->dparams); opts->dparams = NULL; opts->dparam_count = 0; -} \ No newline at end of file +} diff --git a/tests/test_client_cli.c b/tests/test_client_cli.c index 5bbbcef..cca77ee 100644 --- a/tests/test_client_cli.c +++ b/tests/test_client_cli.c @@ -104,18 +104,18 @@ static void test_cli_help() { config_delete(cfg); } -/* Test that --archive's config bundle matches rsync -rlptgoD semantics: - * links + metadata + devices + specials, and NOT compression/multithreading. */ +/* Test that the -a short spelling applies --archive's config bundle, matching + * rsync -rlptgoD semantics: links + metadata + devices + specials, and NOT + * compression/multithreading. (--archive itself is covered by + * test_parse_args_archive; this guards the short alias.) */ static void test_cli_archive_flags() { Config* cfg = config_create(); EXPECT_NOT_NULL(cfg); + char* argv[] = {"fastsync", "-a", "/src", "/dst"}; + int positional_args[2]; + int positional_count = 0; - /* Simulate the --archive flag's implied bundle. */ - cfg->follow_symlinks = true; - cfg->use_metadata = true; - cfg->preserve_devices = true; - cfg->preserve_specials = true; - + EXPECT_EQ_INT(parse_args(cfg, 4, argv, positional_args, &positional_count), 0); EXPECT_TRUE(cfg->follow_symlinks); EXPECT_TRUE(cfg->use_metadata); EXPECT_TRUE(cfg->preserve_devices); @@ -3045,13 +3045,30 @@ static void test_parse_args_block_size() { EXPECT_EQ_INT(parse_args(cfg, 4, argv_eq, positional_args, &positional_count), 0); EXPECT_EQ_INT((int)cfg->delta_block_size, 8192); - /* Out of range: parsed, warned, and the default is kept. */ + cfg->delta_block_size = DELTA_BLOCK_SIZE_DEFAULT; + char* argv_delta_eq[] = {"fastsync", "--delta-block=1024", "/src", "/dst"}; + positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_delta_eq, positional_args, &positional_count), 0); + EXPECT_EQ_INT((int)cfg->delta_block_size, 1024); + + /* Out of range: parsed, warned, and the default is kept (both spellings). */ cfg->delta_block_size = DELTA_BLOCK_SIZE_DEFAULT; char* argv_bad[] = {"fastsync", "--block-size", "1", "/src", "/dst"}; positional_count = 0; EXPECT_EQ_INT(parse_args(cfg, 5, argv_bad, positional_args, &positional_count), 0); EXPECT_EQ_INT((int)cfg->delta_block_size, (int)DELTA_BLOCK_SIZE_DEFAULT); + cfg->delta_block_size = DELTA_BLOCK_SIZE_DEFAULT; + char* argv_bad_inline[] = {"fastsync", "--delta-block=999999", "/src", "/dst"}; + positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_bad_inline, positional_args, &positional_count), 0); + EXPECT_EQ_INT((int)cfg->delta_block_size, (int)DELTA_BLOCK_SIZE_DEFAULT); + + /* A non-numeric value is a hard error for both spellings. */ + char* argv_nan[] = {"fastsync", "--delta-block=abc", "/src", "/dst"}; + positional_count = 0; + EXPECT_EQ_INT(parse_args(cfg, 4, argv_nan, positional_args, &positional_count), -1); + /* A non-default block size changes the number of signature blocks for identical data: block_count = ceil(size / block_size). */ const char data[10000] = {0}; diff --git a/tests/test_server_cli.c b/tests/test_server_cli.c index 5eea11c..c057ebd 100644 --- a/tests/test_server_cli.c +++ b/tests/test_server_cli.c @@ -164,19 +164,26 @@ static void test_server_cli_invalid() { } static void test_server_cli_password_and_early_input() { - const char* args[] = {"s", "--daemon", "--password-file=/etc/fast.pw", "--early-input", - "/run/secrets"}; + const char* args[] = {"s", + "--daemon", + "--password-file=/etc/fast.pw", + "--early-input", + "/run/secrets", + "--iconv=utf-8"}; ServerCliOptions opts; - EXPECT_EQ_INT(parse_ok(args, 5, &opts), 0); + EXPECT_EQ_INT(parse_ok(args, 6, &opts), 0); EXPECT_EQ_STR(opts.password_file, "/etc/fast.pw"); EXPECT_EQ_STR(opts.early_input_file, "/run/secrets"); + EXPECT_EQ_STR(opts.iconv_spec, "utf-8"); - const char* args2[] = {"s", "--daemon", "--password-file", "/etc/fast.pw", - "--early-input=/secrets"}; + const char* args2[] = { + "s", "--daemon", "--password-file", "/etc/fast.pw", "--early-input=/secrets", + "--iconv", "utf-8,iso-8859-1"}; ServerCliOptions opts2; - EXPECT_EQ_INT(parse_ok(args2, 5, &opts2), 0); + EXPECT_EQ_INT(parse_ok(args2, 7, &opts2), 0); EXPECT_EQ_STR(opts2.password_file, "/etc/fast.pw"); EXPECT_EQ_STR(opts2.early_input_file, "/secrets"); + EXPECT_EQ_STR(opts2.iconv_spec, "utf-8,iso-8859-1"); server_cli_options_free(&opts); server_cli_options_free(&opts2); }