Unify parsing logic for command-line integer options

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

diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 4bde16fb4bd465d12452403767320b1f28e751f4..ee8aa71bdf3123b28b23fa523d5f74edb803f596 100644 (file)
--- 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 <limits.h>
 #include <time.h>
 
 #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 8bb0acf498eb7095cc993e5a6f1301c1c7b7ccf9..9ea98481d8fccf5b946715d43f05f8c72bf32ebe 100644 (file)
--- 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 c1334fad357105c130f1fc433620999bf4662e48..4474273daf42b0f799012c035456e45cc7cce2ec 100644 (file)
--- 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 <dirent.h>
+#include <limits.h>
 #include <signal.h>
 #include <sys/stat.h>
 #include <unistd.h>
@@ -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 76bd153fac2413f897c3ab35c9209d6d3566193e..1d59bf37440a1c9f7c473ee41ac5c2ec1fdeeb09 100644 (file)
--- 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 <dirent.h>
+#include <limits.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #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 ba62406105d1a6ef5d7db78b1f6aa12f430de07a..4d1f0fc3a031f73bf908be9730d66548bb7ded27 100644 (file)
--- 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 3c326906e21ed610e590ae35d83b73de1c52526d..e833c5d75e7e38d1bb7657b380111e997cd63027 100644 (file)
--- 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 <dirent.h>
+#include <limits.h>
 #include <time.h>
 #include <sys/stat.h>
 #include <unistd.h>
@@ -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 0fbf736c811ccb139dadeb6b830de285e62635ad..0b39285a015618260c3e2d40c5830c5fd440147d 100644 (file)
--- 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 34b91bb226c433a801cc9258ef7fb498d5e1cc60..90ac445bcdb862555dc10d8648045af900fc4f61 100644 (file)
--- 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 589b4aed539888e29fc7f88e712e0483c8402058..64aaa80eeeec5a8879a8aeda0174e826d6865802 100644 (file)
--- 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 9f12ca6c51da864d64a849da0af147b6d4afa4e7..59de6df7bad32938667747e75c218b0f9f2a80b8 100644 (file)
--- 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 364b5a2e47d01885d746fb83ed15798a9696d523..c51ebb8e31d31638981f5d2e3fec6d0bfd19e131 100644 (file)
--- 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 346a2667fcacf633f34fd5802cbe6818451e3f80..da38d9b1d21ede96d7689dcbb8915898d5907777 100644 (file)
--- 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 ef7e0e549fbf2c16a75838185caa31ea12802387..35b5ccaa5b3b15f9ba1a6e0d8e2a6b476aa7264d 100644 (file)
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -11,6 +11,9 @@
  */
 
 #include "postgres_fe.h"
+
+#include <limits.h>
+
 #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 fc0681538a9b28632f0b27c28811455053764a9a..a0b0250c499571305c7da82ef377d934ec32d038 100644 (file)
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -11,6 +11,8 @@
 
 #include "postgres_fe.h"
 
+#include <limits.h>
+
 #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 a85919c5c19c6c3dd8ed94978cd90efd3c5abc0e..f40bd148579f6498f22e14b7cd6fd8df4e130491 100644 (file)
--- 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 <limits.h>
 
+#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 e19a495dba7d627cddc882f3be7f5e69b11980f3..3e7e512ad9112502bd3b2c3a4c95fed6cda631b9 100644 (file)
--- 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 d653cb94e34e4760dddaeb9c17c529a7d4169f03..e999d56ec0f066f110fa9d0f3e514dc20203df87 100644 (file)
--- 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 */