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 */
