Re: Change atoi to strtol in same place

Alvaro Herrera from 2ndQuadrant wrote:
> Okay, so who is submitting a new version here?  Surafel, Joe?

I've attached a revision that uses pg_strtoint64 from str2int-10.patch
(posted in <alpine.DEB.2.21.1909032007230.21690@lancre>). My patch is
based off that one and c5bc7050af.

It covers the same front-end utilities as atoi-to-strtol-v2.patch. Other
utilities in the pg codebase still have atoi inside, but I thought I'd
share my progress to see if people like the direction. If so, I can
update the rest of the utils.

I added a helper function to a new file in src/fe_utils, but had to
modify Makefiles in ways that might not be too clean. Maybe there's a
better place for the function.

-- 
Joe Nelson      https://begriffs.com
diff --git a/contrib/pg_standby/Makefile b/contrib/pg_standby/Makefile
index 0bca2f8e9e..cb9292d0f4 100644
--- a/contrib/pg_standby/Makefile
+++ b/contrib/pg_standby/Makefile
@@ -6,6 +6,8 @@ PGAPPICON = win32
 PROGRAM = pg_standby
 OBJS    = pg_standby.o $(WIN32RES)
 
+PG_LDFLAGS += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..ce2735f3ae 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -33,6 +33,7 @@
 #include "pg_getopt.h"
 
 #include "access/xlog_internal.h"
+#include "fe_utils/arg_utils.h"
 
 const char *progname;
 
@@ -678,6 +679,10 @@ main(int argc, char **argv)
 
     while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1)
     {
+        pg_strtoint_status s;
+        int64        parsed;
+        char       *parse_error;
+
         switch (c)
         {
             case 'c':            /* Use copy */
@@ -687,12 +692,15 @@ main(int argc, char **argv)
                 debug = true;
                 break;
             case 'k':            /* keepfiles */
-                keepfiles = atoi(optarg);
-                if (keepfiles < 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        0, INT_MAX, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname);
+                    fprintf(stderr, "%s: -k keepfiles %s\n",
+                            progname, parse_error);
                     exit(2);
                 }
+                keepfiles = parsed;
                 break;
             case 'l':            /* Use link */
 
@@ -706,31 +714,39 @@ main(int argc, char **argv)
 #endif
                 break;
             case 'r':            /* Retries */
-                maxretries = atoi(optarg);
-                if (maxretries < 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        0, INT_MAX, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname);
+                    fprintf(stderr, "%s: -r maxretries %s\n",
+                            progname, parse_error);
                     exit(2);
                 }
+                maxretries = parsed;
                 break;
             case 's':            /* Sleep time */
-                sleeptime = atoi(optarg);
-                if (sleeptime <= 0 || sleeptime > 60)
+                s = pg_strtoint64_range(optarg, &parsed, 1, 60, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname);
+                    fprintf(stderr, "%s: -s sleeptime %s\n",
+                            progname, parse_error);
                     exit(2);
                 }
+                sleeptime = parsed;
                 break;
             case 't':            /* Trigger file */
                 triggerPath = pg_strdup(optarg);
                 break;
             case 'w':            /* Max wait time */
-                maxwaittime = atoi(optarg);
-                if (maxwaittime < 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        0, INT_MAX, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname);
+                    fprintf(stderr, "%s: -w maxwaittime %s\n",
+                            progname, parse_error);
                     exit(2);
                 }
+                maxwaittime = parsed;
                 break;
             default:
                 fprintf(stderr, "Try \"%s --help\" for more information.\n", progname);
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 7986872f10..45ec25c6e0 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/arg_utils.h"
 #include "fe_utils/string_utils.h"
 #include "getopt_long.h"
 #include "libpq-fe.h"
@@ -2229,6 +2230,10 @@ main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
                             long_options, &option_index)) != -1)
     {
+        pg_strtoint_status s;
+        int64        parsed;
+        char       *parse_error;
+
         switch (c)
         {
             case 'C':
@@ -2313,12 +2318,13 @@ main(int argc, char **argv)
 #endif
                 break;
             case 'Z':
-                compresslevel = atoi(optarg);
-                if (compresslevel < 0 || compresslevel > 9)
+                s = pg_strtoint64_range(optarg, &parsed, 0, 9, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("invalid compression level \"%s\"", optarg);
+                    pg_log_error("invalid compression level: %s", parse_error);
                     exit(1);
                 }
+                compresslevel = parsed;
                 break;
             case 'c':
                 if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2351,12 +2357,14 @@ main(int argc, char **argv)
                 dbgetpassword = 1;
                 break;
             case 's':
-                standby_message_timeout = atoi(optarg) * 1000;
-                if (standby_message_timeout < 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        0, INT_MAX / 1000, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("invalid status interval \"%s\"", optarg);
+                    pg_log_error("invalid status interval: %s", parse_error);
                     exit(1);
                 }
+                standby_message_timeout = parsed * 1000;
                 break;
             case 'v':
                 verbose++;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index f39c1339d7..2f8f3b0dfe 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -21,6 +21,7 @@
 
 #include "common/file_perm.h"
 #include "common/logging.h"
+#include "fe_utils/arg_utils.h"
 #include "libpq-fe.h"
 #include "access/xlog_internal.h"
 #include "getopt_long.h"
@@ -492,7 +493,6 @@ main(int argc, char **argv)
         {"no-sync", no_argument, NULL, 5},
         {NULL, 0, NULL, 0}
     };
-
     int            c;
     int            option_index;
     char       *db_name;
@@ -521,6 +521,10 @@ main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "D:d:E:h:p:U:s:S:nwWvZ:",
                             long_options, &option_index)) != -1)
     {
+        pg_strtoint_status s;
+        int64        parsed;
+        char       *parse_error;
+
         switch (c)
         {
             case 'D':
@@ -533,11 +537,14 @@ main(int argc, char **argv)
                 dbhost = pg_strdup(optarg);
                 break;
             case 'p':
-                if (atoi(optarg) <= 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        1, (1 << 16) - 1, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("invalid port number \"%s\"", optarg);
+                    pg_log_error("invalid port number: %s", parse_error);
                     exit(1);
                 }
+                /* validated conversion above, but using the string */
                 dbport = pg_strdup(optarg);
                 break;
             case 'U':
@@ -550,12 +557,14 @@ main(int argc, char **argv)
                 dbgetpassword = 1;
                 break;
             case 's':
-                standby_message_timeout = atoi(optarg) * 1000;
-                if (standby_message_timeout < 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        0, INT_MAX / 1000, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("invalid status interval \"%s\"", optarg);
+                    pg_log_error("invalid status interval: %s", parse_error);
                     exit(1);
                 }
+                standby_message_timeout = parsed * 1000;
                 break;
             case 'S':
                 replication_slot = pg_strdup(optarg);
@@ -575,12 +584,13 @@ main(int argc, char **argv)
                 verbose++;
                 break;
             case 'Z':
-                compresslevel = atoi(optarg);
-                if (compresslevel < 0 || compresslevel > 9)
+                s = pg_strtoint64_range(optarg, &parsed, 0, 9, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("invalid compression level \"%s\"", optarg);
+                    pg_log_error("invalid compression level: %s", parse_error);
                     exit(1);
                 }
+                compresslevel = parsed;
                 break;
 /* action */
             case 1:
diff --git a/src/bin/pg_ctl/Makefile b/src/bin/pg_ctl/Makefile
index 83cbf97ed8..f7d375f869 100644
--- a/src/bin/pg_ctl/Makefile
+++ b/src/bin/pg_ctl/Makefile
@@ -24,6 +24,7 @@ LDFLAGS_INTERNAL += $(libpq_pgport)
 SUBMAKE_LIBPQ := submake-libpq
 endif
 
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils
 OBJS=    pg_ctl.o $(WIN32RES)
 
 all: pg_ctl
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index dd76be6dd2..053d79caae 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -28,6 +28,7 @@
 #include "common/file_perm.h"
 #include "common/logging.h"
 #include "common/string.h"
+#include "fe_utils/arg_utils.h"
 #include "getopt_long.h"
 #include "utils/pidfile.h"
 
@@ -2332,6 +2333,10 @@ main(int argc, char **argv)
         while ((c = getopt_long(argc, argv, "cD:e:l:m:N:o:p:P:sS:t:U:wW",
                                 long_options, &option_index)) != -1)
         {
+            pg_strtoint_status s;
+            int64        parsed;
+            char       *parse_error;
+
             switch (c)
             {
                 case 'D':
@@ -2395,7 +2400,14 @@ main(int argc, char **argv)
 #endif
                     break;
                 case 't':
-                    wait_seconds = atoi(optarg);
+                    s = pg_strtoint64_range(optarg, &parsed,
+                                            1, INT_MAX, &parse_error);
+                    if (s != PG_STRTOINT_OK)
+                    {
+                        write_stderr(_("invalid timeout: %s\n"), parse_error);
+                        exit(1);
+                    }
+                    wait_seconds = parsed;
                     wait_seconds_arg = true;
                     break;
                 case 'U':
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7ec0c84540..ab324853b2 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -62,6 +62,7 @@
 #include "pg_backup_db.h"
 #include "pg_backup_utils.h"
 #include "pg_dump.h"
+#include "fe_utils/arg_utils.h"
 #include "fe_utils/connect.h"
 #include "fe_utils/string_utils.h"
 
@@ -430,6 +431,10 @@ main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "abBcCd:E:f:F:h:j:n:N:Op:RsS:t:T:U:vwWxZ:",
                             long_options, &optindex)) != -1)
     {
+        pg_strtoint_status s;
+        int64        parsed;
+        char       *parse_error;
+
         switch (c)
         {
             case 'a':            /* Dump data only */
@@ -473,7 +478,14 @@ main(int argc, char **argv)
                 break;
 
             case 'j':            /* number of dump jobs */
-                numWorkers = atoi(optarg);
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        1, INT_MAX, &parse_error);
+                if (s != PG_STRTOINT_OK)
+                {
+                    pg_log_error("invalid job count: %s", parse_error);
+                    exit_nicely(1);
+                }
+                numWorkers = parsed;
                 break;
 
             case 'n':            /* include schema(s) */
@@ -536,12 +548,13 @@ main(int argc, char **argv)
                 break;
 
             case 'Z':            /* Compression Level */
-                compressLevel = atoi(optarg);
-                if (compressLevel < 0 || compressLevel > 9)
+                s = pg_strtoint64_range(optarg, &parsed, 0, 9, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("compression level must be in range 0..9");
+                    pg_log_error("invalid compression level: %s", parse_error);
                     exit_nicely(1);
                 }
+                compressLevel = parsed;
                 break;
 
             case 0:
@@ -574,12 +587,13 @@ 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)
+                s = pg_strtoint64_range(optarg, &parsed, -15, 3, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_log_error("extra_float_digits must be in range -15..3");
+                    pg_log_error("invalid extra_float_digits: %s", parse_error);
                     exit_nicely(1);
                 }
+                extra_float_digits = parsed;
                 break;
 
             case 9:                /* inserts */
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 40a6b3745c..99f0b8509c 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -45,6 +45,7 @@
 #include <termios.h>
 #endif
 
+#include "fe_utils/arg_utils.h"
 #include "getopt_long.h"
 
 #include "dumputils.h"
@@ -153,6 +154,10 @@ main(int argc, char **argv)
     while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
                             cmdopts, NULL)) != -1)
     {
+        pg_strtoint_status s;
+        int64        parsed;
+        char       *parse_error;
+
         switch (c)
         {
             case 'a':            /* Dump data only */
@@ -183,7 +188,14 @@ main(int argc, char **argv)
                 break;
 
             case 'j':            /* number of restore jobs */
-                numWorkers = atoi(optarg);
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        1, INT_MAX, &parse_error);
+                if (s != PG_STRTOINT_OK)
+                {
+                    pg_log_error("invalid job count: %s", parse_error);
+                    exit_nicely(1);
+                }
+                numWorkers = parsed;
                 break;
 
             case 'l':            /* Dump the TOC summary */
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 28ff4c48ed..e0dd11c9bf 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -14,6 +14,7 @@
 #include <io.h>
 #endif
 
+#include "fe_utils/arg_utils.h"
 #include "getopt_long.h"
 #include "common/string.h"
 #include "utils/pidfile.h"
@@ -106,6 +107,10 @@ parseCommandLine(int argc, char *argv[])
     while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
                                  long_options, &optindex)) != -1)
     {
+        pg_strtoint_status s;
+        int64        parsed;
+        char       *parse_error;
+
         switch (option)
         {
             case 'b':
@@ -129,7 +134,14 @@ parseCommandLine(int argc, char *argv[])
                 break;
 
             case 'j':
-                user_opts.jobs = atoi(optarg);
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        1, INT_MAX, &parse_error);
+                if (s != PG_STRTOINT_OK)
+                {
+                    pg_fatal("invalid job count: %s\n", parse_error);
+                    exit(1);
+                }
+                user_opts.jobs = parsed;
                 break;
 
             case 'k':
@@ -168,19 +180,25 @@ parseCommandLine(int argc, char *argv[])
                  * supported on all old/new versions (added in PG 9.2).
                  */
             case 'p':
-                if ((old_cluster.port = atoi(optarg)) <= 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        1, (1 << 16) - 1, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_fatal("invalid old port number\n");
+                    pg_fatal("invalid old port number: %s\n", parse_error);
                     exit(1);
                 }
+                old_cluster.port = parsed;
                 break;
 
             case 'P':
-                if ((new_cluster.port = atoi(optarg)) <= 0)
+                s = pg_strtoint64_range(optarg, &parsed,
+                                        1, (1 << 16) - 1, &parse_error);
+                if (s != PG_STRTOINT_OK)
                 {
-                    pg_fatal("invalid new port number\n");
+                    pg_fatal("invalid new port number: %s\n", parse_error);
                     exit(1);
                 }
+                new_cluster.port = parsed;
                 break;
 
             case 'r':
diff --git a/src/common/Makefile b/src/common/Makefile
index 2f22b9b101..5c8d6007b8 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -60,8 +60,8 @@ endif
 
 # A few files are currently only built for frontend, not server
 # (Mkvcbuild.pm has a copy of this list, too)
-OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o \
-    logging.o restricted_token.o
+OBJS_FRONTEND = $(OBJS_COMMON) fe_argutils.o fe_memutils.o \
+    file_utils.o logging.o restricted_token.o
 
 # foo.o, foo_shlib.o, and foo_srv.o are all built from foo.c
 OBJS_SHLIB = $(OBJS_FRONTEND:%.o=%_shlib.o)
diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index 7d73800323..21b1f01788 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -19,7 +19,7 @@ include $(top_builddir)/src/Makefile.global
 
 override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
 
-OBJS = mbprint.o print.o psqlscan.o simple_list.o string_utils.o conditional.o
+OBJS = arg_utils.o mbprint.o print.o psqlscan.o simple_list.o string_utils.o conditional.o
 
 all: libpgfeutils.a
 
diff --git a/src/fe_utils/arg_utils.c b/src/fe_utils/arg_utils.c
new file mode 100644
index 0000000000..9c7e4360b4
--- /dev/null
+++ b/src/fe_utils/arg_utils.c
@@ -0,0 +1,46 @@
+/*-------------------------------------------------------------------------
+ *
+ * fe_argutils.c
+ *      argument parsing helpers for frontend code
+ *
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ *
+ * IDENTIFICATION
+ *      src/fe_utils/arg_utils.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres_fe.h"
+
+#include "fe_utils/arg_utils.h"
+
+pg_strtoint_status
+pg_strtoint64_range(const char *str, int64 *result,
+                    int64 min, int64 max, char **error)
+{
+    int64 temp;
+    pg_strtoint_status s = pg_strtoint64(str, &temp);
+
+    if (s == PG_STRTOINT_OK && (temp < min || temp > max))
+        s = PG_STRTOINT_RANGE_ERROR;
+
+    switch (s)
+    {
+    case PG_STRTOINT_OK:
+        *result = temp;
+        break;
+    case PG_STRTOINT_SYNTAX_ERROR:
+        *error = psprintf("could not parse '%s' as integer", str);
+        break;
+    case PG_STRTOINT_RANGE_ERROR:
+        *error = psprintf("%s is outside range "
+                          INT64_FORMAT ".." INT64_FORMAT,
+                          str, min, max);
+        break;
+    default:
+        pg_unreachable();
+    }
+    return s;
+}
diff --git a/src/include/fe_utils/arg_utils.h b/src/include/fe_utils/arg_utils.h
new file mode 100644
index 0000000000..64d413e148
--- /dev/null
+++ b/src/include/fe_utils/arg_utils.h
@@ -0,0 +1,26 @@
+/*
+ *    fe_argutils.h
+ *        argument parsing helpers for frontend code
+ *
+ *    Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ *    src/include/fe_utils/arg_utils.h
+ */
+#ifndef FE_ARGUTILS_H
+#define FE_ARGUTILS_H
+
+#include "common/string.h"
+
+/*
+ * Parses string as int64 like pg_strtoint64, but fails
+ * with PG_STRTOINT_RANGE_ERROR if the result is outside
+ * the range min .. max inclusive.
+ *
+ * On failure, creates user-friendly error message with
+ * psprintf, and assigns it to the error output parameter.
+ */
+pg_strtoint_status
+pg_strtoint64_range(const char *str, int64 *result,
+                    int64 min, int64 max, char **error);
+
+#endif                            /* FE_ARGUTILS_H */