From 6f164e6d17616a157ea5d9e34dbb1b211c080c41 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Sat, 24 Jul 2021 18:35:03 +0900 Subject: [PATCH] Unify parsing logic for command-line integer options MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Most of the integer options for command-line binaries now make use of a single routine able to do the job, fixing issues with the detection of sloppy values caused for example by the use of atoi(), that fails on strings beginning with numerical characters with junk trailing characters. This commit cuts down the number of strings requiring translation by 26 per my count, switching the code to have two error types for invalid and out-of-range values instead. Much more could be done here, with float or even int64 options, but int32 was the most appealing case as it is possible to rely on strtol() to do the job reliably. Note that there are some exceptions for now, like pg_ctl or pg_upgrade that use their own logging logic. A couple of negative TAP tests required some adjustments for the new errors generated. pg_dump and pg_restore tracked the maximum number of parallel jobs within the option parsing. The code is refactored a bit to track that in the code dedicated to parallelism instead. Author: Kyotaro Horiguchi, Michael Paquier Reviewed-by: David Rowley, Álvaro Herrera Discussion: https://postgr.es/m/CALj2ACXqdG9WhqVoJ9zYf-iZt7sgK7Szv5USs=he6NnWQ2ofTA@mail.gmail.com --- src/bin/pg_amcheck/pg_amcheck.c | 8 ++- src/bin/pg_basebackup/pg_basebackup.c | 17 +++--- src/bin/pg_basebackup/pg_receivewal.c | 23 +++------ src/bin/pg_basebackup/pg_recvlogical.c | 25 ++++----- src/bin/pg_checksums/Makefile | 3 ++ src/bin/pg_checksums/pg_checksums.c | 9 ++-- src/bin/pg_dump/parallel.h | 13 +++++ src/bin/pg_dump/pg_dump.c | 47 ++++------------- src/bin/pg_dump/pg_restore.c | 22 ++------ src/bin/pg_dump/t/001_basic.pl | 20 ++++---- src/bin/pgbench/pgbench.c | 60 +++++++--------------- src/bin/pgbench/t/002_pgbench_no_server.pl | 34 +++++++----- src/bin/scripts/createuser.c | 13 ++--- src/bin/scripts/reindexdb.c | 9 ++-- src/bin/scripts/vacuumdb.c | 31 ++++------- src/fe_utils/option_utils.c | 39 ++++++++++++++ src/include/fe_utils/option_utils.h | 3 ++ 17 files changed, 177 insertions(+), 199 deletions(-) diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index 4bde16fb4bd..ee8aa71bdf3 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -12,6 +12,7 @@ */ #include "postgres_fe.h" +#include #include #include "catalog/pg_am_d.h" @@ -326,12 +327,9 @@ main(int argc, char *argv[]) append_btree_pattern(&opts.exclude, optarg, encoding); break; case 'j': - opts.jobs = atoi(optarg); - if (opts.jobs < 1) - { - pg_log_error("number of parallel jobs must be at least 1"); + if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX, + &opts.jobs)) exit(1); - } break; case 'p': port = pg_strdup(optarg); diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 8bb0acf498e..9ea98481d8f 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -31,6 +31,7 @@ #include "common/file_utils.h" #include "common/logging.h" #include "common/string.h" +#include "fe_utils/option_utils.h" #include "fe_utils/recovery_gen.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" @@ -2371,12 +2372,9 @@ main(int argc, char **argv) #endif break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) - { - pg_log_error("invalid compression level \"%s\"", optarg); + if (!option_parse_int(optarg, "-Z/--compress", 0, 9, + &compresslevel)) exit(1); - } break; case 'c': if (pg_strcasecmp(optarg, "fast") == 0) @@ -2409,12 +2407,11 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) - { - pg_log_error("invalid status interval \"%s\"", optarg); + if (!option_parse_int(optarg, "-s/--status-interval", 0, + INT_MAX / 1000, + &standby_message_timeout)) exit(1); - } + standby_message_timeout *= 1000; break; case 'v': verbose++; diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index c1334fad357..4474273daf4 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -15,6 +15,7 @@ #include "postgres_fe.h" #include +#include #include #include #include @@ -22,6 +23,7 @@ #include "access/xlog_internal.h" #include "common/file_perm.h" #include "common/logging.h" +#include "fe_utils/option_utils.h" #include "getopt_long.h" #include "libpq-fe.h" #include "receivelog.h" @@ -532,11 +534,6 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) - { - pg_log_error("invalid port number \"%s\"", optarg); - exit(1); - } dbport = pg_strdup(optarg); break; case 'U': @@ -549,12 +546,11 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) - { - pg_log_error("invalid status interval \"%s\"", optarg); + if (!option_parse_int(optarg, "-s/--status-interval", 0, + INT_MAX / 1000, + &standby_message_timeout)) exit(1); - } + standby_message_timeout *= 1000; break; case 'S': replication_slot = pg_strdup(optarg); @@ -574,12 +570,9 @@ main(int argc, char **argv) verbose++; break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) - { - pg_log_error("invalid compression level \"%s\"", optarg); + if (!option_parse_int(optarg, "-Z/--compress", 0, 9, + &compresslevel)) exit(1); - } break; /* action */ case 1: diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index 76bd153fac2..1d59bf37440 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -13,6 +13,7 @@ #include "postgres_fe.h" #include +#include #include #include #ifdef HAVE_SYS_SELECT_H @@ -23,6 +24,7 @@ #include "common/fe_memutils.h" #include "common/file_perm.h" #include "common/logging.h" +#include "fe_utils/option_utils.h" #include "getopt_long.h" #include "libpq-fe.h" #include "libpq/pqsignal.h" @@ -739,12 +741,11 @@ main(int argc, char **argv) outfile = pg_strdup(optarg); break; case 'F': - fsync_interval = atoi(optarg) * 1000; - if (fsync_interval < 0) - { - pg_log_error("invalid fsync interval \"%s\"", optarg); + if (!option_parse_int(optarg, "-F/--fsync-interval", 0, + INT_MAX / 1000, + &fsync_interval)) exit(1); - } + fsync_interval *= 1000; break; case 'n': noloop = 1; @@ -763,11 +764,6 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) - { - pg_log_error("invalid port number \"%s\"", optarg); - exit(1); - } dbport = pg_strdup(optarg); break; case 'U': @@ -820,12 +816,11 @@ main(int argc, char **argv) plugin = pg_strdup(optarg); break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) - { - pg_log_error("invalid status interval \"%s\"", optarg); + if (!option_parse_int(optarg, "-s/--status-interval", 0, + INT_MAX / 1000, + &standby_message_timeout)) exit(1); - } + standby_message_timeout *= 1000; break; case 'S': replication_slot = pg_strdup(optarg); diff --git a/src/bin/pg_checksums/Makefile b/src/bin/pg_checksums/Makefile index ba62406105d..4d1f0fc3a03 100644 --- a/src/bin/pg_checksums/Makefile +++ b/src/bin/pg_checksums/Makefile @@ -15,6 +15,9 @@ subdir = src/bin/pg_checksums top_builddir = ../../.. include $(top_builddir)/src/Makefile.global +# We need libpq only because fe_utils does. +LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport) + OBJS = \ $(WIN32RES) \ pg_checksums.o diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 3c326906e21..e833c5d75e7 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -15,6 +15,7 @@ #include "postgres_fe.h" #include +#include #include #include #include @@ -24,6 +25,7 @@ #include "common/file_perm.h" #include "common/file_utils.h" #include "common/logging.h" +#include "fe_utils/option_utils.h" #include "getopt_long.h" #include "pg_getopt.h" #include "storage/bufpage.h" @@ -518,11 +520,10 @@ main(int argc, char *argv[]) mode = PG_MODE_ENABLE; break; case 'f': - if (atoi(optarg) == 0) - { - pg_log_error("invalid filenode specification, must be numeric: %s", optarg); + if (!option_parse_int(optarg, "-f/--filenode", 0, + INT_MAX, + NULL)) exit(1); - } only_filenode = pstrdup(optarg); break; case 'N': diff --git a/src/bin/pg_dump/parallel.h b/src/bin/pg_dump/parallel.h index 0fbf736c811..0b39285a015 100644 --- a/src/bin/pg_dump/parallel.h +++ b/src/bin/pg_dump/parallel.h @@ -33,6 +33,19 @@ typedef enum WFW_ALL_IDLE } WFW_WaitOption; +/* + * Maximum number of parallel jobs allowed. + * + * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually) + * parallel jobs because that's the maximum limit for the + * WaitForMultipleObjects() call. + */ +#ifdef WIN32 +#define PG_MAX_JOBS MAXIMUM_WAIT_OBJECTS +#else +#define PG_MAX_JOBS INT_MAX +#endif + /* ParallelSlot is an opaque struct known only within parallel.c */ typedef struct ParallelSlot ParallelSlot; diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 34b91bb226c..90ac445bcdb 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -56,6 +56,7 @@ #include "catalog/pg_type_d.h" #include "common/connect.h" #include "dumputils.h" +#include "fe_utils/option_utils.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" #include "libpq/libpq-fs.h" @@ -322,14 +323,12 @@ main(int argc, char **argv) DumpableObject *boundaryObjs; int i; int optindex; - char *endptr; RestoreOptions *ropt; Archive *fout; /* the script file */ bool g_verbose = false; const char *dumpencoding = NULL; const char *dumpsnapshot = NULL; char *use_role = NULL; - long rowsPerInsert; int numWorkers = 1; int compressLevel = -1; int plainText = 0; @@ -487,7 +486,10 @@ main(int argc, char **argv) break; case 'j': /* number of dump jobs */ - numWorkers = atoi(optarg); + if (!option_parse_int(optarg, "-j/--jobs", 1, + PG_MAX_JOBS, + &numWorkers)) + exit_nicely(1); break; case 'n': /* include schema(s) */ @@ -550,12 +552,9 @@ main(int argc, char **argv) break; case 'Z': /* Compression Level */ - compressLevel = atoi(optarg); - if (compressLevel < 0 || compressLevel > 9) - { - pg_log_error("compression level must be in range 0..9"); + if (!option_parse_int(optarg, "-Z/--compress", 0, 9, + &compressLevel)) exit_nicely(1); - } break; case 0: @@ -588,12 +587,9 @@ main(int argc, char **argv) case 8: have_extra_float_digits = true; - extra_float_digits = atoi(optarg); - if (extra_float_digits < -15 || extra_float_digits > 3) - { - pg_log_error("extra_float_digits must be in range -15..3"); + if (!option_parse_int(optarg, "--extra-float-digits", -15, 3, + &extra_float_digits)) exit_nicely(1); - } break; case 9: /* inserts */ @@ -607,18 +603,9 @@ main(int argc, char **argv) break; case 10: /* rows per insert */ - errno = 0; - rowsPerInsert = strtol(optarg, &endptr, 10); - - if (endptr == optarg || *endptr != '\0' || - rowsPerInsert <= 0 || rowsPerInsert > INT_MAX || - errno == ERANGE) - { - pg_log_error("rows-per-insert must be in range %d..%d", - 1, INT_MAX); + if (!option_parse_int(optarg, "--rows-per-insert", 1, INT_MAX, + &dopt.dump_inserts)) exit_nicely(1); - } - dopt.dump_inserts = (int) rowsPerInsert; break; case 11: /* include foreign data */ @@ -720,18 +707,6 @@ main(int argc, char **argv) if (!plainText) dopt.outputCreateDB = 1; - /* - * On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually) - * parallel jobs because that's the maximum limit for the - * WaitForMultipleObjects() call. - */ - if (numWorkers <= 0 -#ifdef WIN32 - || numWorkers > MAXIMUM_WAIT_OBJECTS -#endif - ) - fatal("invalid number of parallel jobs"); - /* Parallel backup only in the directory archive format so far */ if (archiveFormat != archDirectory && numWorkers > 1) fatal("parallel backup only supported by the directory format"); diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index 589b4aed539..64aaa80eeee 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -46,6 +46,7 @@ #endif #include "dumputils.h" +#include "fe_utils/option_utils.h" #include "getopt_long.h" #include "parallel.h" #include "pg_backup_utils.h" @@ -181,7 +182,10 @@ main(int argc, char **argv) break; case 'j': /* number of restore jobs */ - numWorkers = atoi(optarg); + if (!option_parse_int(optarg, "-j/--jobs", 1, + PG_MAX_JOBS, + &numWorkers)) + exit(1); break; case 'l': /* Dump the TOC summary */ @@ -344,22 +348,6 @@ main(int argc, char **argv) exit_nicely(1); } - if (numWorkers <= 0) - { - pg_log_error("invalid number of parallel jobs"); - exit(1); - } - - /* See comments in pg_dump.c */ -#ifdef WIN32 - if (numWorkers > MAXIMUM_WAIT_OBJECTS) - { - pg_log_error("maximum number of parallel jobs is %d", - MAXIMUM_WAIT_OBJECTS); - exit(1); - } -#endif - /* Can't do single-txn mode with multiple connections */ if (opts->single_txn && numWorkers > 1) { diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl index 9f12ca6c51d..59de6df7bad 100644 --- a/src/bin/pg_dump/t/001_basic.pl +++ b/src/bin/pg_dump/t/001_basic.pl @@ -103,8 +103,8 @@ command_fails_like( command_fails_like( [ 'pg_dump', '-j', '-1' ], - qr/\Qpg_dump: error: invalid number of parallel jobs\E/, - 'pg_dump: invalid number of parallel jobs'); + qr/\Qpg_dump: error: -j\/--jobs must be in range\E/, + 'pg_dump: -j/--jobs must be in range'); command_fails_like( [ 'pg_dump', '-F', 'garbage' ], @@ -113,8 +113,8 @@ command_fails_like( command_fails_like( [ 'pg_restore', '-j', '-1', '-f -' ], - qr/\Qpg_restore: error: invalid number of parallel jobs\E/, - 'pg_restore: invalid number of parallel jobs'); + qr/\Qpg_restore: error: -j\/--jobs must be in range\E/, + 'pg_restore: -j/--jobs must be in range'); command_fails_like( [ 'pg_restore', '--single-transaction', '-j3', '-f -' ], @@ -123,18 +123,18 @@ command_fails_like( command_fails_like( [ 'pg_dump', '-Z', '-1' ], - qr/\Qpg_dump: error: compression level must be in range 0..9\E/, - 'pg_dump: compression level must be in range 0..9'); + qr/\Qpg_dump: error: -Z\/--compress must be in range 0..9\E/, + 'pg_dump: -Z/--compress must be in range'); command_fails_like( [ 'pg_dump', '--extra-float-digits', '-16' ], - qr/\Qpg_dump: error: extra_float_digits must be in range -15..3\E/, - 'pg_dump: extra_float_digits must be in range -15..3'); + qr/\Qpg_dump: error: --extra-float-digits must be in range\E/, + 'pg_dump: --extra-float-digits must be in range'); command_fails_like( [ 'pg_dump', '--rows-per-insert', '0' ], - qr/\Qpg_dump: error: rows-per-insert must be in range 1..2147483647\E/, - 'pg_dump: rows-per-insert must be in range 1..2147483647'); + qr/\Qpg_dump: error: --rows-per-insert must be in range\E/, + 'pg_dump: --rows-per-insert must be in range'); command_fails_like( [ 'pg_restore', '--if-exists', '-f -' ], diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 364b5a2e47d..c51ebb8e31d 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -63,6 +63,7 @@ #include "common/username.h" #include "fe_utils/cancel.h" #include "fe_utils/conditional.h" +#include "fe_utils/option_utils.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -5887,10 +5888,9 @@ main(int argc, char **argv) break; case 'c': benchmarking_option_set = true; - nclients = atoi(optarg); - if (nclients <= 0) + if (!option_parse_int(optarg, "-c/--clients", 1, INT_MAX, + &nclients)) { - pg_log_fatal("invalid number of clients: \"%s\"", optarg); exit(1); } #ifdef HAVE_GETRLIMIT @@ -5914,10 +5914,9 @@ main(int argc, char **argv) break; case 'j': /* jobs */ benchmarking_option_set = true; - nthreads = atoi(optarg); - if (nthreads <= 0) + if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX, + &nthreads)) { - pg_log_fatal("invalid number of threads: \"%s\"", optarg); exit(1); } #ifndef ENABLE_THREAD_SAFETY @@ -5938,30 +5937,21 @@ main(int argc, char **argv) break; case 's': scale_given = true; - scale = atoi(optarg); - if (scale <= 0) - { - pg_log_fatal("invalid scaling factor: \"%s\"", optarg); + if (!option_parse_int(optarg, "-s/--scale", 1, INT_MAX, + &scale)) exit(1); - } break; case 't': benchmarking_option_set = true; - nxacts = atoi(optarg); - if (nxacts <= 0) - { - pg_log_fatal("invalid number of transactions: \"%s\"", optarg); + if (!option_parse_int(optarg, "-t/--transactions", 1, INT_MAX, + &nxacts)) exit(1); - } break; case 'T': benchmarking_option_set = true; - duration = atoi(optarg); - if (duration <= 0) - { - pg_log_fatal("invalid duration: \"%s\"", optarg); + if (!option_parse_int(optarg, "-T/--time", 1, INT_MAX, + &duration)) exit(1); - } break; case 'U': username = pg_strdup(optarg); @@ -6019,12 +6009,9 @@ main(int argc, char **argv) break; case 'F': initialization_option_set = true; - fillfactor = atoi(optarg); - if (fillfactor < 10 || fillfactor > 100) - { - pg_log_fatal("invalid fillfactor: \"%s\"", optarg); + if (!option_parse_int(optarg, "-F/--fillfactor", 10, 100, + &fillfactor)) exit(1); - } break; case 'M': benchmarking_option_set = true; @@ -6039,12 +6026,9 @@ main(int argc, char **argv) break; case 'P': benchmarking_option_set = true; - progress = atoi(optarg); - if (progress <= 0) - { - pg_log_fatal("invalid thread progress delay: \"%s\"", optarg); + if (!option_parse_int(optarg, "-P/--progress", 1, INT_MAX, + &progress)) exit(1); - } break; case 'R': { @@ -6098,12 +6082,9 @@ main(int argc, char **argv) break; case 5: /* aggregate-interval */ benchmarking_option_set = true; - agg_interval = atoi(optarg); - if (agg_interval <= 0) - { - pg_log_fatal("invalid number of seconds for aggregation: \"%s\"", optarg); + if (!option_parse_int(optarg, "--aggregate-interval", 1, INT_MAX, + &agg_interval)) exit(1); - } break; case 6: /* progress-timestamp */ progress_timestamp = true; @@ -6135,12 +6116,9 @@ main(int argc, char **argv) break; case 11: /* partitions */ initialization_option_set = true; - partitions = atoi(optarg); - if (partitions < 0) - { - pg_log_fatal("invalid number of partitions: \"%s\"", optarg); + if (!option_parse_int(optarg, "--partitions", 1, INT_MAX, + &partitions)) exit(1); - } break; case 12: /* partition-method */ initialization_option_set = true; diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl index 346a2667fca..da38d9b1d21 100644 --- a/src/bin/pgbench/t/002_pgbench_no_server.pl +++ b/src/bin/pgbench/t/002_pgbench_no_server.pl @@ -37,7 +37,7 @@ sub pgbench_scripts local $Test::Builder::Level = $Test::Builder::Level + 1; my ($opts, $stat, $out, $err, $name, $files) = @_; - my @cmd = ('pgbench', split /\s+/, $opts); + my @cmd = ('pgbench', split /\s+/, $opts); my @filenames = (); if (defined $files) { @@ -91,17 +91,26 @@ my @options = ( [qr{weight spec.* out of range .*: -1}] ], [ 'too many scripts', '-S ' x 129, [qr{at most 128 SQL scripts}] ], - [ 'bad #clients', '-c three', [qr{invalid number of clients: "three"}] ], [ - 'bad #threads', '-j eleven', [qr{invalid number of threads: "eleven"}] + 'bad #clients', '-c three', + [qr{invalid value "three" for option -c/--clients}] + ], + [ + 'bad #threads', '-j eleven', + [qr{invalid value "eleven" for option -j/--jobs}] + ], + [ + 'bad scale', '-i -s two', + [qr{invalid value "two" for option -s/--scale}] ], - [ 'bad scale', '-i -s two', [qr{invalid scaling factor: "two"}] ], [ 'invalid #transactions', - '-t zil', - [qr{invalid number of transactions: "zil"}] + '-t zil', [qr{invalid value "zil" for option -t/--transactions}] + ], + [ + 'invalid duration', + '-T ten', [qr{invalid value "ten" for option -T/--time}] ], - [ 'invalid duration', '-T ten', [qr{invalid duration: "ten"}] ], [ '-t XOR -T', '-N -l --aggregate-interval=5 --log-prefix=notused -t 1000 -T 1', @@ -113,11 +122,11 @@ my @options = ( [qr{specify either }] ], [ 'bad variable', '--define foobla', [qr{invalid variable definition}] ], - [ 'invalid fillfactor', '-F 1', [qr{invalid fillfactor}] ], + [ 'invalid fillfactor', '-F 1', [qr{-F/--fillfactor must be in range}] ], [ 'invalid query mode', '-M no-such-mode', [qr{invalid query mode}] ], [ 'invalid progress', '--progress=0', - [qr{invalid thread progress delay}] + [qr{-P/--progress must be in range}] ], [ 'invalid rate', '--rate=0.0', [qr{invalid rate limit}] ], [ 'invalid latency', '--latency-limit=0.0', [qr{invalid latency limit}] ], @@ -126,8 +135,9 @@ my @options = ( [qr{invalid sampling rate}] ], [ - 'invalid aggregate interval', '--aggregate-interval=-3', - [qr{invalid .* seconds for}] + 'invalid aggregate interval', + '--aggregate-interval=-3', + [qr{--aggregate-interval must be in range}] ], [ 'weight zero', @@ -171,7 +181,7 @@ my @options = ( [ 'bad partition number', '-i --partitions -1', - [qr{invalid number of partitions: "-1"}] + [qr{--partitions must be in range}] ], [ 'partition method without partitioning', diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c index ef7e0e549fb..35b5ccaa5b3 100644 --- a/src/bin/scripts/createuser.c +++ b/src/bin/scripts/createuser.c @@ -11,6 +11,9 @@ */ #include "postgres_fe.h" + +#include + #include "common.h" #include "common/logging.h" #include "common/string.h" @@ -89,8 +92,6 @@ main(int argc, char *argv[]) while ((c = getopt_long(argc, argv, "h:p:U:g:wWedDsSrRiIlLc:PE", long_options, &optindex)) != -1) { - char *endptr; - switch (c) { case 'h': @@ -145,13 +146,9 @@ main(int argc, char *argv[]) login = TRI_NO; break; case 'c': - conn_limit = strtol(optarg, &endptr, 10); - if (*endptr != '\0' || conn_limit < -1) /* minimum valid value */ - { - pg_log_error("invalid value for --connection-limit: %s", - optarg); + if (!option_parse_int(optarg, "-c/--connection-limit", + -1, INT_MAX, &conn_limit)) exit(1); - } break; case 'P': pwprompt = true; diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index fc0681538a9..a0b0250c499 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -11,6 +11,8 @@ #include "postgres_fe.h" +#include + #include "catalog/pg_class_d.h" #include "common.h" #include "common/connect.h" @@ -151,12 +153,9 @@ main(int argc, char *argv[]) simple_string_list_append(&indexes, optarg); break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) - { - pg_log_error("number of parallel jobs must be at least 1"); + if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX, + &concurrentCons)) exit(1); - } break; case 'v': verbose = true; diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index a85919c5c19..f40bd148579 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -12,8 +12,9 @@ #include "postgres_fe.h" -#include "catalog/pg_class_d.h" +#include +#include "catalog/pg_class_d.h" #include "common.h" #include "common/connect.h" #include "common/logging.h" @@ -192,20 +193,14 @@ main(int argc, char *argv[]) vacopts.verbose = true; break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) - { - pg_log_error("number of parallel jobs must be at least 1"); + if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX, + &concurrentCons)) exit(1); - } break; case 'P': - vacopts.parallel_workers = atoi(optarg); - if (vacopts.parallel_workers < 0) - { - pg_log_error("parallel workers for vacuum must be greater than or equal to zero"); + if (!option_parse_int(optarg, "-P/--parallel", 0, INT_MAX, + &vacopts.parallel_workers)) exit(1); - } break; case 2: maintenance_db = pg_strdup(optarg); @@ -220,20 +215,14 @@ main(int argc, char *argv[]) vacopts.skip_locked = true; break; case 6: - vacopts.min_xid_age = atoi(optarg); - if (vacopts.min_xid_age <= 0) - { - pg_log_error("minimum transaction ID age must be at least 1"); + if (!option_parse_int(optarg, "--min-xid-age", 1, INT_MAX, + &vacopts.min_xid_age)) exit(1); - } break; case 7: - vacopts.min_mxid_age = atoi(optarg); - if (vacopts.min_mxid_age <= 0) - { - pg_log_error("minimum multixact ID age must be at least 1"); + if (!option_parse_int(optarg, "--min-mxid-age", 1, INT_MAX, + &vacopts.min_mxid_age)) exit(1); - } break; case 8: vacopts.no_index_cleanup = true; diff --git a/src/fe_utils/option_utils.c b/src/fe_utils/option_utils.c index e19a495dba7..3e7e512ad91 100644 --- a/src/fe_utils/option_utils.c +++ b/src/fe_utils/option_utils.c @@ -12,6 +12,8 @@ #include "postgres_fe.h" +#include "common/logging.h" +#include "common/string.h" #include "fe_utils/option_utils.h" /* @@ -36,3 +38,40 @@ handle_help_version_opts(int argc, char *argv[], } } } + +/* + * option_parse_int + * + * Parse integer value for an option. If the parsing is successful, returns + * true and stores the result in *result if that's given; if parsing fails, + * returns false. + */ +bool +option_parse_int(const char *optarg, const char *optname, + int min_range, int max_range, + int *result) +{ + char *endptr; + int val; + + errno = 0; + val = strtoint(optarg, &endptr, 10); + + if (*endptr) + { + pg_log_error("invalid value \"%s\" for option %s", + optarg, optname); + return false; + } + + if (errno == ERANGE || val < min_range || val > max_range) + { + pg_log_error("%s must be in range %d..%d", + optname, min_range, max_range); + return false; + } + + if (result) + *result = val; + return true; +} diff --git a/src/include/fe_utils/option_utils.h b/src/include/fe_utils/option_utils.h index d653cb94e34..e999d56ec0f 100644 --- a/src/include/fe_utils/option_utils.h +++ b/src/include/fe_utils/option_utils.h @@ -19,5 +19,8 @@ typedef void (*help_handler) (const char *progname); extern void handle_help_version_opts(int argc, char *argv[], const char *fixed_progname, help_handler hlp); +extern bool option_parse_int(const char *optarg, const char *optname, + int min_range, int max_range, + int *result); #endif /* OPTION_UTILS_H */ -- 2.30.2