Re: Change atoi to strtol in same place - Mailing list pgsql-hackers
From | Joe Nelson |
---|---|
Subject | Re: Change atoi to strtol in same place |
Date | |
Msg-id | 20190910040249.GA25585@begriffs.com Whole thread Raw |
In response to | Re: Change atoi to strtol in same place (Alvaro Herrera from 2ndQuadrant <alvherre@alvh.no-ip.org>) |
Responses |
Re: Change atoi to strtol in same place
|
List | pgsql-hackers |
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 */
pgsql-hackers by date: