Thread: Change atoi to strtol in same place
Hello,
we use atoi for user argument processing in same place which return zero for both invalid input and input value zero. In most case its ok because we error out with appropriate error message for input zero but in same place where we accept zero as valued input it case a problem by preceding for invalid input as input value zero. The attached patch change those place to strtol which can handle invalid input
regards
Surafel
Attachment
Hi Surafel, On Mon, Jul 01, 2019 at 08:48:27PM +0300, Surafel Temesgen wrote: >Hello, > >we use atoi for user argument processing in same place which return zero >for both invalid input and input value zero. In most case its ok because we >error out with appropriate error message for input zero but in same place >where we accept zero as valued input it case a problem by preceding for >invalid input as input value zero. The attached patch change those place to >strtol which can handle invalid input > >regards > >Surafel This seems to have bit-rotted (due to minor changes to pg_basebackup). Can you fix that and post an updated version? In general, I think it's a good idea to fix those places, but I wonder if we need to change the error messages too. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> Surafel Temesgen wrote: > > we use atoi for user argument processing in same place which return > > zero for both invalid input and input value zero. [...] in same > > place where we accept zero as valued input it case a problem by > > preceding for invalid input as input value zero. strtol which can > > handle invalid input Not only that, but atoi causes Undefined Behavior on erroneous input. The C99 standard says this: 7.20.1 Numeric conversion functions The functions atof, atoi, atol, and atoll need not affect the value of the integer expression errno on an error. If the value of the result cannot be represented, the behavior is undefined. Tomas Vondra wrote: > This seems to have bit-rotted (due to minor changes to pg_basebackup). > Can you fix that and post an updated version? I adjusted the patch to apply cleanly on a0555ddab9. > In general, I think it's a good idea to fix those places, but I wonder > if we need to change the error messages too. I'll leave that decision for the community to debate. I did, however, remove the newlines for the new error messages being passed to pg_log_error(). As discussed in message [0], the logging functions in common/logging.c now contain an assertion that messages do not end in newline: Assert(fmt[strlen(fmt) - 1] != '\n'); (in pg_log_error via pg_log_generic via pg_log_generic_v) I also added limits.h to some places it was missing, so the patch would build. 0: https://postgr.es/m/6a609b43-4f57-7348-6480-bd022f924310@2ndquadrant.com diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c index 23f706b21d..2bcacbfbb5 100644 --- a/contrib/pg_standby/pg_standby.c +++ b/contrib/pg_standby/pg_standby.c @@ -27,6 +27,7 @@ #include <dirent.h> #include <sys/stat.h> #include <fcntl.h> +#include <limits.h> #include <signal.h> #include <sys/time.h> @@ -638,6 +639,14 @@ sigquit_handler(int sig) int main(int argc, char **argv) { + char *keepfilesendptr; + char *maxretriesendptr; + char *sleeptimeendptr; + char *maxwaittimeendptr; + long numkeepfiles; + long nummaxretries; + long numsleeptime; + long nummaxwaittime; int c; progname = get_progname(argv[0]); @@ -688,12 +697,17 @@ main(int argc, char **argv) debug = true; break; case 'k': /* keepfiles */ - keepfiles = atoi(optarg); - if (keepfiles < 0) + errno = 0; + numkeepfiles = strtol(optarg, &keepfilesendptr, 10); + + if (keepfilesendptr == optarg || *keepfilesendptr != '\0' || + numkeepfiles < 0 || numkeepfiles > INT_MAX || + errno == ERANGE) { - fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname); + fprintf(stderr, "%s: -k keepfiles must be in range %d..%d\n", progname, 0, INT_MAX); exit(2); } + keepfiles = (int) numkeepfiles; break; case 'l': /* Use link */ @@ -707,31 +721,46 @@ main(int argc, char **argv) #endif break; case 'r': /* Retries */ - maxretries = atoi(optarg); - if (maxretries < 0) + errno = 0; + nummaxretries = strtol(optarg, &maxretriesendptr, 10); + + if (maxretriesendptr == optarg || *maxretriesendptr != '\0' || + nummaxretries < 0 || nummaxretries > INT_MAX || + errno == ERANGE) { - fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname); + fprintf(stderr, "%s: -r maxretries must be in range %d..%d\n", progname, 0, INT_MAX); exit(2); } + maxretries = (int) nummaxretries; break; case 's': /* Sleep time */ - sleeptime = atoi(optarg); - if (sleeptime <= 0 || sleeptime > 60) + errno = 0; + numsleeptime = strtol(optarg, &sleeptimeendptr, 10); + + if (sleeptimeendptr == optarg || *sleeptimeendptr != '\0' || + numsleeptime <= 0 || numsleeptime > 60 || + errno == ERANGE) { - fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname); + fprintf(stderr, "%s: -s sleeptime must be in range %d..%d\n", progname, 1, 59); exit(2); } + sleeptime = (int) numsleeptime; break; case 't': /* Trigger file */ triggerPath = pg_strdup(optarg); break; case 'w': /* Max wait time */ - maxwaittime = atoi(optarg); - if (maxwaittime < 0) + errno = 0; + nummaxwaittime = strtol(optarg, &maxwaittimeendptr, 10); + + if (maxwaittimeendptr == optarg || *maxwaittimeendptr != '\0' || + nummaxwaittime < 0 || nummaxwaittime > INT_MAX || + errno == ERANGE) { - fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname); + fprintf(stderr, "%s: -w maxwaittime must be in range %d..%d\n", progname, 0, INT_MAX); exit(2); } + maxwaittime = (int) nummaxwaittime; 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 77a7c148ba..6ed523fdd6 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -15,6 +15,7 @@ #include <unistd.h> #include <dirent.h> +#include <limits.h> #include <sys/stat.h> #include <sys/wait.h> #include <signal.h> @@ -2197,6 +2198,10 @@ main(int argc, char **argv) {"no-verify-checksums", no_argument, NULL, 3}, {NULL, 0, NULL, 0} }; + char *compressEndptr; + char *timeoutEndptr; + long compressNumber; + long timeoutNumber; int c; int option_index; @@ -2309,12 +2314,18 @@ main(int argc, char **argv) #endif break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) + errno = 0; + compressNumber = strtol(optarg, &compressEndptr, 10); + + if (compressEndptr == optarg || *compressEndptr != '\0' || + compressNumber < 0 || compressNumber > 9 || + errno == ERANGE) { - pg_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("compression level must be in range %d..%d \"%s\"", + 0, 9, optarg); exit(1); } + compresslevel = (int) compressNumber; break; case 'c': if (pg_strcasecmp(optarg, "fast") == 0) @@ -2347,12 +2358,18 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + timeoutNumber = strtol(optarg, &timeoutEndptr, 10); + + if (timeoutEndptr == optarg || *timeoutEndptr != '\0' || + timeoutNumber < 0 || timeoutNumber > INT_MAX || + errno == ERANGE) { - pg_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("status interval must be in range %d..%d \"%s\"", + 0, INT_MAX, optarg); exit(1); } + standby_message_timeout = (int) timeoutNumber * 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..94a20f50a2 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 <dirent.h> +#include <limits.h> #include <signal.h> #include <sys/stat.h> #include <unistd.h> @@ -492,10 +493,15 @@ main(int argc, char **argv) {"no-sync", no_argument, NULL, 5}, {NULL, 0, NULL, 0} }; - + long compressNumber; + long timeoutNumber; + long portNumber; int c; int option_index; + char *compressEndptr; char *db_name; + char *timeoutEndptr; + char *portEndptr; uint32 hi, lo; @@ -533,9 +539,15 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) + errno = 0; + portNumber = strtol(optarg, &portEndptr, 10); + + if (portEndptr == optarg || *portEndptr != '\0' || + portNumber <= 0 || portNumber > INT_MAX || + errno == ERANGE) { - pg_log_error("invalid port number \"%s\"", optarg); + pg_log_error("port number must be in range %d..%d \"%s\"", + 1, INT_MAX, optarg); exit(1); } dbport = pg_strdup(optarg); @@ -550,12 +562,18 @@ main(int argc, char **argv) dbgetpassword = 1; break; case 's': - standby_message_timeout = atoi(optarg) * 1000; - if (standby_message_timeout < 0) + errno = 0; + timeoutNumber = strtol(optarg, &timeoutEndptr, 10); + + if (timeoutEndptr == optarg || *timeoutEndptr != '\0' || + timeoutNumber < 0 || timeoutNumber > INT_MAX || + errno == ERANGE) { - pg_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("status interval must be in range %d..%d \"%s\"", + 0, INT_MAX, optarg); exit(1); } + standby_message_timeout = (int) timeoutNumber * 1000; break; case 'S': replication_slot = pg_strdup(optarg); @@ -575,12 +593,18 @@ main(int argc, char **argv) verbose++; break; case 'Z': - compresslevel = atoi(optarg); - if (compresslevel < 0 || compresslevel > 9) + errno = 0; + compressNumber = strtol(optarg, &compressEndptr, 10); + + if (compressEndptr == optarg || *compressEndptr != '\0' || + compressNumber < 0 || compressNumber > 9 || + errno == ERANGE) { - pg_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("compression level must be in range %d..%d \"%s\"", + 0, 9, optarg); exit(1); } + compresslevel = (int) compressNumber; break; /* action */ case 1: diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c index a10bc8d545..04f2072bb5 100644 --- a/src/bin/pg_ctl/pg_ctl.c +++ b/src/bin/pg_ctl/pg_ctl.c @@ -12,6 +12,7 @@ #include "postgres_fe.h" #include <fcntl.h> +#include <limits.h> #include <signal.h> #include <time.h> #include <sys/stat.h> @@ -2266,6 +2267,8 @@ main(int argc, char **argv) }; char *env_wait; + char *seconds_endptr; + long seconds; int option_index; int c; pgpid_t killproc = 0; @@ -2395,7 +2398,18 @@ main(int argc, char **argv) #endif break; case 't': - wait_seconds = atoi(optarg); + errno = 0; + seconds = strtol(optarg, &seconds_endptr, 10); + + if (seconds_endptr == optarg || *seconds_endptr != '\0' || + seconds <= 0 || seconds > INT_MAX || + errno == ERANGE) + { + write_stderr(_("timeout must be in range %d..%d\n"), + 1, INT_MAX); + exit(1); + } + wait_seconds = (int) seconds; 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 bbf44a1820..577b6554cc 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -311,14 +311,20 @@ main(int argc, char **argv) DumpableObject *boundaryObjs; int i; int optindex; + char *compressEndptr; + char *digitsEndptr; char *endptr; + char *workersEndptr; RestoreOptions *ropt; Archive *fout; /* the script file */ bool g_verbose = false; const char *dumpencoding = NULL; const char *dumpsnapshot = NULL; char *use_role = NULL; + long compressNumber; + long floatDigits; long rowsPerInsert; + long workersNumber; int numWorkers = 1; trivalue prompt_password = TRI_DEFAULT; int compressLevel = -1; @@ -473,7 +479,18 @@ main(int argc, char **argv) break; case 'j': /* number of dump jobs */ - numWorkers = atoi(optarg); + errno = 0; + workersNumber = strtol(optarg, &workersEndptr, 10); + + if (workersEndptr == optarg || *workersEndptr != '\0' || + workersNumber <= 0 || workersNumber > INT_MAX || + errno == ERANGE) + { + pg_log_error("jobs must be in range %d..%d", + 1, INT_MAX); + exit_nicely(1); + } + numWorkers = (int) workersNumber; break; case 'n': /* include schema(s) */ @@ -536,12 +553,17 @@ main(int argc, char **argv) break; case 'Z': /* Compression Level */ - compressLevel = atoi(optarg); - if (compressLevel < 0 || compressLevel > 9) + errno = 0; + compressNumber = strtol(optarg, &compressEndptr, 10); + + if (compressEndptr == optarg || *compressEndptr != '\0' || + compressNumber < 0 || compressNumber > 9 || + errno == ERANGE) { pg_log_error("compression level must be in range 0..9"); exit_nicely(1); } + compressLevel = (int) compressNumber; break; case 0: @@ -573,13 +595,18 @@ main(int argc, char **argv) break; case 8: - have_extra_float_digits = true; - extra_float_digits = atoi(optarg); - if (extra_float_digits < -15 || extra_float_digits > 3) + errno = 0; + floatDigits = strtol(optarg, &digitsEndptr, 10); + + if (digitsEndptr == optarg || *digitsEndptr != '\0' || + floatDigits < -15 || floatDigits > 3 || + errno == ERANGE) { pg_log_error("extra_float_digits must be in range -15..3"); exit_nicely(1); } + have_extra_float_digits = true; + extra_float_digits = (int) floatDigits; break; case 9: /* inserts */ diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c index f9b1ae6809..8f52bd7b3b 100644 --- a/src/bin/pg_dump/pg_restore.c +++ b/src/bin/pg_dump/pg_restore.c @@ -41,6 +41,7 @@ #include "postgres_fe.h" #include <ctype.h> +#include <limits.h> #ifdef HAVE_TERMIOS_H #include <termios.h> #endif @@ -60,9 +61,11 @@ int main(int argc, char **argv) { RestoreOptions *opts; + long workersNumber; int c; int exit_code; int numWorkers = 1; + char *workersEndptr; Archive *AH; char *inputFileSpec; static int disable_triggers = 0; @@ -185,7 +188,18 @@ main(int argc, char **argv) break; case 'j': /* number of restore jobs */ - numWorkers = atoi(optarg); + errno = 0; + workersNumber = strtol(optarg, &workersEndptr, 10); + + if (workersEndptr == optarg || *workersEndptr != '\0' || + workersNumber <= 0 || workersNumber > INT_MAX || + errno == ERANGE) + { + pg_log_error("jobs must be in range %d..%d", + 1, INT_MAX); + exit_nicely(1); + } + numWorkers = (int) workersNumber; break; case 'l': /* Dump the TOC summary */ diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c index d76f27c9e8..0a3ec2c653 100644 --- a/src/bin/pg_upgrade/option.c +++ b/src/bin/pg_upgrade/option.c @@ -9,6 +9,7 @@ #include "postgres_fe.h" +#include <limits.h> #include <time.h> #ifdef WIN32 #include <io.h> @@ -59,11 +60,17 @@ parseCommandLine(int argc, char *argv[]) {NULL, 0, NULL, 0} }; + long workersNumber; + long newPortNumber; + long oldNumber; int option; /* Command line option */ int optindex = 0; /* used by getopt_long */ int os_user_effective_id; FILE *fp; char **filename; + char *newPortEndptr; + char *oldPortEndptr; + char *workersEndptr; time_t run_time = time(NULL); user_opts.transfer_mode = TRANSFER_MODE_COPY; @@ -127,7 +134,18 @@ parseCommandLine(int argc, char *argv[]) break; case 'j': - user_opts.jobs = atoi(optarg); + errno = 0; + workersNumber = strtol(optarg, &workersEndptr, 10); + + if (workersEndptr == optarg || *workersEndptr != '\0' || + workersNumber <= 0 || workersNumber > INT_MAX || + errno == ERANGE) + { + pg_fatal("jobs must be in range %d..%d", + 0, INT_MAX); + exit(1); + } + user_opts.jobs = (int) workersNumber; break; case 'k': @@ -166,19 +184,31 @@ 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) + errno = 0; + oldNumber = strtol(optarg, &oldPortEndptr, 10); + + if (oldPortEndptr == optarg || *oldPortEndptr != '\0' || + oldNumber <= 0 || oldNumber > INT_MAX || + errno == ERANGE) { pg_fatal("invalid old port number\n"); exit(1); } + old_cluster.port = (int) oldNumber; break; case 'P': - if ((new_cluster.port = atoi(optarg)) <= 0) + errno = 0; + newPortNumber = strtol(optarg, &newPortEndptr, 10); + + if (newPortEndptr == optarg || *newPortEndptr != '\0' || + newPortNumber <= 0 || newPortNumber > INT_MAX || + errno == ERANGE) { pg_fatal("invalid new port number\n"); exit(1); } + new_cluster.port = (int) newPortNumber; break; case 'r':
On Wed, 24 Jul 2019 at 16:02, Joe Nelson <joe@begriffs.com> wrote: > > In general, I think it's a good idea to fix those places, but I wonder > > if we need to change the error messages too. > > I'll leave that decision for the community to debate. I did, however, > remove the newlines for the new error messages being passed to > pg_log_error(). I'd like to put my vote not to add this complex code to each option validation that requires an integer number. I'm not sure there currently is a home for it, but if there was, wouldn't it be better writing a function that takes a lower and upper bound and sets some output param with the value and returns a bool to indicate if it's within range or not? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2019-07-24 16:57:42 +1200, David Rowley wrote: > On Wed, 24 Jul 2019 at 16:02, Joe Nelson <joe@begriffs.com> wrote: > > > In general, I think it's a good idea to fix those places, but I wonder > > > if we need to change the error messages too. > > > > I'll leave that decision for the community to debate. I did, however, > > remove the newlines for the new error messages being passed to > > pg_log_error(). > > I'd like to put my vote not to add this complex code to each option > validation that requires an integer number. I'm not sure there > currently is a home for it, but if there was, wouldn't it be better > writing a function that takes a lower and upper bound and sets some > output param with the value and returns a bool to indicate if it's > within range or not? +many
On Wed, Jul 24, 2019 at 04:57:42PM +1200, David Rowley wrote: > I'd like to put my vote not to add this complex code to each option > validation that requires an integer number. I'm not sure there > currently is a home for it, but if there was, wouldn't it be better > writing a function that takes a lower and upper bound and sets some > output param with the value and returns a bool to indicate if it's > within range or not? Perhaps. When I see this patch calling strtol basically only for 10 as base, this reminds me of Fabien Coelho's patch refactor all the strtoint routines we have in the code: https://commitfest.postgresql.org/23/2099/ The conclusion that we are reaching on the thread is to remove more dependencies on strtol that we have in the code, and replace it with our own, more performant wrappers. This thread makes me wondering that we had better wait before doing this move. -- Michael
Attachment
On 2019-Jul-24, Michael Paquier wrote: > The conclusion that we are reaching on the thread is to remove more > dependencies on strtol that we have in the code, and replace it with > our own, more performant wrappers. This thread makes me wondering > that we had better wait before doing this move. Okay, so who is submitting a new version here? Surafel, Joe? Waiting on Author. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera from 2ndQuadrant wrote: > Okay, so who is submitting a new version here? Surafel, Joe? Sure, I'll look into it over the weekend.
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 */
On Mon, Sep 09, 2019 at 11:02:49PM -0500, Joe Nelson wrote: > 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. Using a wrapper in src/fe_utils makes sense. I would have used options.c for the new file, or would that be too much generic? The code indentation is weird, particularly the switch/case in pg_strtoint64_range(). The error handling is awkward. I think that you should just call pg_log_error in pg_strtoint64_range instead of returning an error string as you do. You could do that by passing down the option name to the routine, and generate a new set of error messages using that. Why do you need to update common/Makefile? The builds on Windows are broken. You are adding one file into fe_utils, but forgot to update @pgfeutilsfiles in Mkvcbuild.pm. -- Michael
Attachment
On Tue, Sep 10, 2019 at 1:36 AM Michael Paquier <michael@paquier.xyz> wrote: > The error handling is awkward. I think that you should just call > pg_log_error in pg_strtoint64_range instead of returning an error > string as you do. You could do that by passing down the option name > to the routine, and generate a new set of error messages using that. -1. I think it's very useful to have routines for this sort of thing that return an error message rather than emitting an error report directly. That gives the caller a lot more control. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Sep 10, 2019 at 08:03:32AM -0400, Robert Haas wrote:
> -1. I think it's very useful to have routines for this sort of thing
> that return an error message rather than emitting an error report
> directly. That gives the caller a lot more control.
Please let me counter-argue here. There are a couple of reasons to
not do as the patch proposes:
1) Consistency with the error messages makes less work for translators,
who already have a lot to deal with. The patch is awkward in this
sense, to give some examples:
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid status interval \"%s\"", optarg);
+ pg_log_error("invalid status interval: %s", parse_error);
}
[...]
- pg_log_error("invalid compression level \"%s\"", optarg);
+ pg_log_error("invalid compression level: %s", parse_error);
2) A centralized error message can provide the same level of details.
Here are suggestions for each error status:
pg_log_error("could not parse value for option %s", optname);
pg_log_error("invalid value for option %s", optname);
optname should be defined by the caller with strings like
"-t/--timeout" or such. Then, if ranges are specified and the error
is on a range, I think that we should just add a second error message
to provide a hint to the user, if wanted by the caller of
pg_strtoint64_range() so an extra argument could do handle that:
pg_log_error("value must be in range %d..%d")
3) I think that we should not expose directly the status values of
pg_strtoint_status in pg_strtoint64_range(), that's less for module
authors to worry about, and that would be the same approach as we are
using for the wrappers of pg_strto[u]intXX() in the patch of the other
thread (see pg_strto[u]intXX_check for example in [1]).
[1]: https://www.postgresql.org/message-id/20190910030525.GA22934@paquier.xyz
--
Michael
Attachment
Michael Paquier wrote: > Using a wrapper in src/fe_utils makes sense. I would have used > options.c for the new file, or would that be too much generic? Sure, options.c sounds fine -- it doesn't seem any more generic than "arg_utils" and is a little simpler. The only other question I have is if the function inside -- with some adjustment in its interface -- might be useful in other contexts beyond front-end args checking. > The code indentation is weird, particularly the switch/case in > pg_strtoint64_range(). I did run pgindent... Do I need to tell it about the existence of the new file? > The error handling is awkward. Let's continue this point in your follow-up <20190911035356.GE1953@paquier.xyz>. > Why do you need to update common/Makefile? Thought I needed to do this so that other parts would link properly, but just removed the changes there and stuff still links OK, so I'll remove that change. > The builds on Windows are broken. You are adding one file into > fe_utils, but forgot to update @pgfeutilsfiles in Mkvcbuild.pm. -- Thanks for the tip, I'm still learning about the build process. Is there a service I can use to test my patches across multiple platforms? I'd rather not bother reviewers with build problems that I can catch in a more automated way. -- Joe Nelson https://begriffs.com
Robert Haas wrote:
> > -1. I think it's very useful to have routines for this sort of thing
> > that return an error message rather than emitting an error report
> > directly. That gives the caller a lot more control.
Michael Paquier wrote:
> 1) Consistency with the error messages makes less work for translators,
> who already have a lot to deal with.
Agreed that the messages can be slightly inconsistent. I tried to make
the new messages match the styles of other messages in their respective
utilities. Maybe the bigger issue here is inconsistent output styles
across the utilities in general:
pg_standby.c includes flag names
%s: -r maxretries %s
pg_basebackup.c writes the settings out in words
invalid compression level: %s
Note that the final %s in those examples will expand to a more detailed
message. For example passing "-Z 10" to pg_dump in the current patch will
output:
pg_dump: error: invalid compression level: 10 is outside range 0..9
> 2) A centralized error message can provide the same level of details.
Even assuming we standardize the message format, different callers have
different means to handle the messages. The front-end utilities affected in my
patch use calls as varied as fprintf, pg_log_error, write_stderr and pg_fatal.
Thus pg_strtoint64_range needs more flexibility than calling pg_log_error
internally.
> 3) I think that we should not expose directly the status values of
> pg_strtoint_status in pg_strtoint64_range(), that's less for module
> authors to worry about, and that would be the same approach as we are
> using for the wrappers of pg_strto[u]intXX() in the patch of the other
> thread (see pg_strto[u]intXX_check for example in [1]).
The pg_strto[u]intXX_check functions can return the integer directly only
because they handle errors with ereport(ERROR, ...). However, as I mentioned
earlier, this is not always what the front-end utilities need to do.
--
Joe Nelson https://begriffs.com
On Tue, Sep 10, 2019 at 11:54 PM Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Sep 10, 2019 at 08:03:32AM -0400, Robert Haas wrote: > > -1. I think it's very useful to have routines for this sort of thing > > that return an error message rather than emitting an error report > > directly. That gives the caller a lot more control. > > Please let me counter-argue here. OK, but on the other hand, Joe's example of a custom message "invalid compression level: 10 is outside range 0..9" is a world better than "invalid compression level: %s". We might even be able to do better "argument to -Z must be a compression level between 0 and 9". In backend error-reporting, it's often important to show the misguided value, because it may be coming from a complex query where it's hard to find the source of the problematic value. But if the user types -Z42 or -Zborked, I'm not sure it's important to tell them that the problem is with "42" or "borked". It's more important to explain the concept, or such would be my judgement. Also, consider an option where the value must be an integer between 1 and 100 or one of several fixed strings (e.g. think of recovery_target_timeline). The user clearly can't use the generic error message in that case. Perhaps the answer is to say that such users shouldn't use the provided range-checking function but rather implement the logic from scratch. But that seems a bit limiting. Also, suppose the user doesn't want to pg_log_error(). Maybe it's a warning. Maybe it doesn't even need to be logged. What this boils down to in the end is that putting more of the policy decisions into the function helps ensure consistency and save code when the function is used, but it also results in the function being used less often. Reasonable people can differ on the merits of different approaches, but for me the down side of returning the error message appears minor at most, and the up sides seem significant. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
... can we have a new patch? Not only because there seems to have been some discussion points that have gone unaddressed (?), but also because Appveyor complains really badly about this one. https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.58672 -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera wrote: > ... can we have a new patch? Not only because there seems to have > been some discussion points that have gone unaddressed (?) Yes, I'll work on it over the weekend. > but also because Appveyor complains really badly about this one. > https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.58672 Note that it requires functions from str2int-10.patch, and will not compile when applied to master by itself. I didn't want to duplicate functionality from that other uncommitted patch in mine.
On Fri, Sep 27, 2019 at 09:35:53PM -0500, Joe Nelson wrote: > Note that it requires functions from str2int-10.patch, and will not > compile when applied to master by itself. I didn't want to duplicate > functionality from that other uncommitted patch in mine. If we have a dependency between both threads, perhaps more people could comment there? Here is the most relevant update: https://www.postgresql.org/message-id/20190917022913.GB1660@paquier.xyz -- Michael
Attachment
Alvaro Herrera wrote: > ... can we have a new patch? OK, I've attached v4. It works cleanly on 55282fa20f with str2int-16.patch applied. My patch won't compile without the other one applied too. Changed: [x] revert my changes in common/Makefile [x] rename arg_utils.[ch] to option.[ch] [x] update @pgfeutilsfiles in Mkvcbuild.pm [x] pgindent everything [x] get rid of atoi() in more utilities One question about how the utilities parse port numbers. I currently have it check that the value can be parsed as an integer, and that its range is within 1 .. (1<<16)-1. I wonder if the former restriction is (un)desirable, because ultimately getaddrinfo() takes a "service name description" for the port, which can be a name such as found in '/etc/services' as well as the string representation of a number. If desired, I *could* treat only range errors as a failure for ports, and allow integer parse errors. -- 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..56ac7fd726 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/option.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 55ef13926d..7869c8cf9a 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -32,6 +32,7 @@ #include "common/logging.h" #include "common/string.h" #include "fe_utils/recovery_gen.h" +#include "fe_utils/option.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -2073,6 +2074,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': @@ -2157,12 +2162,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) @@ -2195,12 +2201,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..8c6924e1d1 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/option.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_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index af29dd7651..fe0612fc1f 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -26,6 +26,7 @@ #include "common/file_perm.h" #include "common/fe_memutils.h" #include "common/logging.h" +#include "fe_utils/option.h" #include "getopt_long.h" #include "libpq-fe.h" #include "libpq/pqsignal.h" @@ -705,6 +706,10 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:", long_options, &option_index)) != -1) { + pg_strtoint_status s; + int64 parsed; + char *parse_error; + switch (c) { /* general options */ @@ -712,12 +717,14 @@ main(int argc, char **argv) outfile = pg_strdup(optarg); break; case 'F': - fsync_interval = atoi(optarg) * 1000; - if (fsync_interval < 0) + s = pg_strtoint64_range(optarg, &parsed, + 0, INT_MAX / 1000, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("invalid fsync interval \"%s\"", optarg); + pg_log_error("invalid fsync interval: %s", parse_error); exit(1); } + fsync_interval = parsed * 1000; break; case 'n': noloop = 1; @@ -733,11 +740,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': @@ -790,12 +800,14 @@ main(int argc, char **argv) plugin = pg_strdup(optarg); 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); 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..ad03a0d080 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/option.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 f01fea5b91..265e88fbab 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/option.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..b01c169c14 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/option.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..9b99ad3bf6 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/option.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/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 600f1deb71..7eb3a6ff63 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -36,6 +36,7 @@ #include "common/logging.h" #include "common/string.h" #include "fe_utils/conditional.h" +#include "fe_utils/option.h" #include "getopt_long.h" #include "libpq-fe.h" #include "portability/instr_time.h" @@ -5094,6 +5095,9 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1) { char *script; + pg_strtoint_status s; + int64 parsed; + char *parse_error; switch (c) { @@ -5125,13 +5129,15 @@ main(int argc, char **argv) break; case 'c': benchmarking_option_set = true; - nclients = atoi(optarg); - if (nclients <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of clients: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of clients: %s\n", + parse_error); exit(1); } + nclients = parsed; #ifdef HAVE_GETRLIMIT #ifdef RLIMIT_NOFILE /* most platforms use RLIMIT_NOFILE */ if (getrlimit(RLIMIT_NOFILE, &rlim) == -1) @@ -5153,13 +5159,15 @@ main(int argc, char **argv) break; case 'j': /* jobs */ benchmarking_option_set = true; - nthreads = atoi(optarg); - if (nthreads <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of threads: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of threads: %s\n", + parse_error); exit(1); } + nthreads = parsed; #ifndef ENABLE_THREAD_SAFETY if (nthreads != 1) { @@ -5178,31 +5186,37 @@ main(int argc, char **argv) break; case 's': scale_given = true; - scale = atoi(optarg); - if (scale <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg); + fprintf(stderr, "invalid scaling factor: %s\n", parse_error); exit(1); } + scale = parsed; break; case 't': benchmarking_option_set = true; - nxacts = atoi(optarg); - if (nxacts <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of transactions: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of transactions: %s\n", + parse_error); exit(1); } + nxacts = parsed; break; case 'T': benchmarking_option_set = true; - duration = atoi(optarg); - if (duration <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { fprintf(stderr, "invalid duration: \"%s\"\n", optarg); exit(1); } + duration = parsed; break; case 'U': login = pg_strdup(optarg); @@ -5261,12 +5275,14 @@ main(int argc, char **argv) break; case 'F': initialization_option_set = true; - fillfactor = atoi(optarg); - if (fillfactor < 10 || fillfactor > 100) + s = pg_strtoint64_range(optarg, &parsed, + 10, 100, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg); + fprintf(stderr, "invalid fillfactor: %s\n", parse_error); exit(1); } + fillfactor = parsed; break; case 'M': benchmarking_option_set = true; @@ -5282,13 +5298,15 @@ main(int argc, char **argv) break; case 'P': benchmarking_option_set = true; - progress = atoi(optarg); - if (progress <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid thread progress delay: \"%s\"\n", - optarg); + fprintf(stderr, "invalid thread progress delay: %s\n", + parse_error); exit(1); } + progress = parsed; break; case 'R': { @@ -5343,13 +5361,15 @@ main(int argc, char **argv) break; case 5: /* aggregate-interval */ benchmarking_option_set = true; - agg_interval = atoi(optarg); - if (agg_interval <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of seconds for aggregation: %s\n", + parse_error); exit(1); } + agg_interval = parsed; break; case 6: /* progress-timestamp */ progress_timestamp = true; diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index f00aec15de..5024aaad67 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -15,6 +15,7 @@ #include "common.h" #include "common/logging.h" #include "fe_utils/connect.h" +#include "fe_utils/option.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" #include "scripts_parallel.h" @@ -105,6 +106,10 @@ main(int argc, char *argv[]) /* process command-line options */ while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1) { + pg_strtoint_status s; + int64 parsed; + char *parse_error; + switch (c) { case 'h': @@ -147,12 +152,14 @@ main(int argc, char *argv[]) simple_string_list_append(&indexes, optarg); break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("invalid number of parallel jobs: %s", parse_error); exit(1); } + concurrentCons = parsed; break; case 'v': verbose = true; diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 2c7219239f..9266966d62 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -17,6 +17,7 @@ #include "common.h" #include "common/logging.h" #include "fe_utils/connect.h" +#include "fe_utils/option.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" #include "scripts_parallel.h" @@ -124,6 +125,10 @@ main(int argc, char *argv[]) while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options, &optindex)) != -1) { + pg_strtoint_status s; + int64 parsed; + char *parse_error; + switch (c) { case 'h': @@ -175,12 +180,14 @@ main(int argc, char *argv[]) vacopts.verbose = true; break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("invalid number of parallel jobs: %s", parse_error); exit(1); } + concurrentCons = parsed; break; case 2: maintenance_db = pg_strdup(optarg); @@ -195,20 +202,24 @@ main(int argc, char *argv[]) vacopts.skip_locked = true; break; case 6: - vacopts.min_xid_age = atoi(optarg); - if (vacopts.min_xid_age <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("minimum transaction ID age must be at least 1"); + pg_log_error("invalid minimum transaction ID age: %s", parse_error); exit(1); } + vacopts.min_xid_age = parsed; break; case 7: - vacopts.min_mxid_age = atoi(optarg); - if (vacopts.min_mxid_age <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("minimum multixact ID age must be at least 1"); + pg_log_error("invalid minimum multixact ID age: %s", parse_error); exit(1); } + vacopts.min_mxid_age = parsed; break; default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile index f2e516a2aa..83063abdcd 100644 --- a/src/fe_utils/Makefile +++ b/src/fe_utils/Makefile @@ -19,8 +19,8 @@ include $(top_builddir)/src/Makefile.global override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -OBJS = conditional.o mbprint.o print.o psqlscan.o recovery_gen.o \ - simple_list.o string_utils.o +OBJS = conditional.o mbprint.o option.o print.o psqlscan.o \ + recovery_gen.o simple_list.o string_utils.o all: libpgfeutils.a diff --git a/src/fe_utils/option.c b/src/fe_utils/option.c new file mode 100644 index 0000000000..b17cfe5e9d --- /dev/null +++ b/src/fe_utils/option.c @@ -0,0 +1,46 @@ +/*------------------------------------------------------------------------- + * + * option.c + * argument parsing helpers for frontend code + * + * Copyright (c) 2019, PostgreSQL Global Development Group + * + * + * IDENTIFICATION + * src/fe_utils/option.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres_fe.h" + +#include "fe_utils/option.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/option.h b/src/include/fe_utils/option.h new file mode 100644 index 0000000000..56c6f5da3f --- /dev/null +++ b/src/include/fe_utils/option.h @@ -0,0 +1,26 @@ +/* + * option.h + * argument parsing helpers for frontend code + * + * Copyright (c) 2019, PostgreSQL Global Development Group + * + * src/include/fe_utils/option.h + */ +#ifndef FE_OPTION_H +#define FE_OPTION_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_OPTION_H */ diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index 7a103e6140..a9a01d22b7 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -142,7 +142,7 @@ sub mkvcbuild our @pgcommonbkndfiles = @pgcommonallfiles; our @pgfeutilsfiles = qw( - conditional.c mbprint.c print.c psqlscan.l psqlscan.c + conditional.c mbprint.c option.c print.c psqlscan.l psqlscan.c simple_list.c string_utils.c recovery_gen.c); $libpgport = $solution->AddProject('libpgport', 'lib', 'misc');
Hello. At Sun, 29 Sep 2019 23:51:23 -0500, Joe Nelson <joe@begriffs.com> wrote in <20190930045123.GC68117@begriffs.com> > Alvaro Herrera wrote: > > ... can we have a new patch? > > OK, I've attached v4. It works cleanly on 55282fa20f with > str2int-16.patch applied. My patch won't compile without the other one > applied too. > > Changed: > [x] revert my changes in common/Makefile > [x] rename arg_utils.[ch] to option.[ch] > [x] update @pgfeutilsfiles in Mkvcbuild.pm > [x] pgindent everything > [x] get rid of atoi() in more utilities Compiler complained as "INT_MAX undeclared" (gcc 7.3 / CentOS7.6). > One question about how the utilities parse port numbers. I currently > have it check that the value can be parsed as an integer, and that its > range is within 1 .. (1<<16)-1. I wonder if the former restriction is > (un)desirable, because ultimately getaddrinfo() takes a "service name > description" for the port, which can be a name such as found in > '/etc/services' as well as the string representation of a number. If > desired, I *could* treat only range errors as a failure for ports, and > allow integer parse errors. We could do that, but perhaps no use for our usage. We are not likely to use named ports other than 'postgres', if any. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Tue, 01 Oct 2019 19:32:08 +0900 (Tokyo Standard Time), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in <20191001.193208.264851337.horikyota.ntt@gmail.com> > Hello. > > At Sun, 29 Sep 2019 23:51:23 -0500, Joe Nelson <joe@begriffs.com> wrote in <20190930045123.GC68117@begriffs.com> > > Alvaro Herrera wrote: > > > ... can we have a new patch? > > > > OK, I've attached v4. It works cleanly on 55282fa20f with > > str2int-16.patch applied. My patch won't compile without the other one > > applied too. > > > > Changed: > > [x] revert my changes in common/Makefile > > [x] rename arg_utils.[ch] to option.[ch] > > [x] update @pgfeutilsfiles in Mkvcbuild.pm > > [x] pgindent everything > > [x] get rid of atoi() in more utilities I didn't checked closely, but -k of pg_standby's message looks somewhat strange. Needs a separator? > pg_standby: -k keepfiles could not parse 'hoge' as integer Building a sentense just concatenating multiple nonindependent (or incomplete) subphrases makes translation harder. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro Horiguchi wrote:
> > pg_standby: -k keepfiles could not parse 'hoge' as integer
>
> I didn't checked closely, but -k of pg_standby's message looks
> somewhat strange. Needs a separator?
Good point, how about this:
pg_standby: -k keepfiles: <localized error message>
> Building a sentense just concatenating multiple nonindependent
> (or incomplete) subphrases makes translation harder.
I could have pg_strtoint64_range() wrap its error messages in _() so
that translators could customize the messages prior to concatenation.
*error = psprintf(_("could not parse '%s' as integer"), str);
Would this suffice?
On 2019-Oct-03, Joe Nelson wrote:
> Kyotaro Horiguchi wrote:
> > > pg_standby: -k keepfiles could not parse 'hoge' as integer
> >
> > I didn't checked closely, but -k of pg_standby's message looks
> > somewhat strange. Needs a separator?
>
> Good point, how about this:
>
> pg_standby: -k keepfiles: <localized error message>
The wording is a bit strange. How about something like
pg_standy: invalid argument to -k: %s
where the %s is the error message produced like you propose:
> I could have pg_strtoint64_range() wrap its error messages in _() so
> that translators could customize the messages prior to concatenation.
>
> *error = psprintf(_("could not parse '%s' as integer"), str);
... except that they would rather be more explicit about what the
problem is. "insufficient digits" or "extraneous character", etc.
> Would this suffice?
I hope that no callers would like to have the messages not translated,
because that seems like it would become a mess.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Joe, On a quick look, the patch seems to be going in a good direction although there are quite some pending work to be done. One suggestion: The start value for port number is set to 1, however it seems like the port number that falls in the range of 1-1023 is reserved and can't be used. So, is it possible to have the start value as 1024 instead of 1 ? Further, I encountered one syntax error (INT_MAX undeclared) as the header file "limits.h" has not been included in postgres_fe.h or option.h -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com On Fri, Oct 4, 2019 at 9:04 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Oct-03, Joe Nelson wrote: > > > Kyotaro Horiguchi wrote: > > > > pg_standby: -k keepfiles could not parse 'hoge' as integer > > > > > > I didn't checked closely, but -k of pg_standby's message looks > > > somewhat strange. Needs a separator? > > > > Good point, how about this: > > > > pg_standby: -k keepfiles: <localized error message> > > The wording is a bit strange. How about something like > pg_standy: invalid argument to -k: %s > > where the %s is the error message produced like you propose: > > > I could have pg_strtoint64_range() wrap its error messages in _() so > > that translators could customize the messages prior to concatenation. > > > > *error = psprintf(_("could not parse '%s' as integer"), str); > > ... except that they would rather be more explicit about what the > problem is. "insufficient digits" or "extraneous character", etc. > > > Would this suffice? > > I hope that no callers would like to have the messages not translated, > because that seems like it would become a mess. > > -- > Álvaro Herrera https://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > >
Ashutosh Sharma wrote:
> One suggestion: The start value for port number is set to 1, however
> it seems like the port number that falls in the range of 1-1023 is
> reserved and can't be used. So, is it possible to have the start value
> as 1024 instead of 1 ?
Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
so the range can be updated in one place for all utilities.
> Further, I encountered one syntax error (INT_MAX undeclared) as the
> header file "limits.h" has not been included in postgres_fe.h or
> option.h
Oops. Added limits.h now in option.h. The Postgres build accidentally
worked on my system without explicitly including this header because
__has_builtin(__builtin_isinf) is true for me so src/include/port.h
pulled in math.h with an #if which pulled in limits.h.
> Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > The wording is a bit strange. How about something like pg_standy:
> > invalid argument to -k: %s
I updated the error messages in pg_standby.
> > > *error = psprintf(_("could not parse '%s' as integer"), str);
> >
> > ... except that they would rather be more explicit about what the
> > problem is. "insufficient digits" or "extraneous character", etc.
Sadly pg_strtoint64 returns the same error code for both cases. So we
could either petition for more detailed errors in the thread for that
other patch, or examine the string ourselves to check. Maybe it's not
needed since "could not parse 'abc' as integer" kind of does show the
problem.
> > I hope that no callers would like to have the messages not translated,
> > because that seems like it would become a mess.
That's true... I think it should be OK though, since we return the
pg_strtoint_status so callers can inspect that rather than relying on certain
words being in the error string. I'm guessing the translated string would be
most appropriate for end users.
--
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..9ce736249b 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/option.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: invalid argument for -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: invalid argument for -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: invalid argument for -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: invalid argument for -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 55ef13926d..7869c8cf9a 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -32,6 +32,7 @@
#include "common/logging.h"
#include "common/string.h"
#include "fe_utils/recovery_gen.h"
+#include "fe_utils/option.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -2073,6 +2074,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':
@@ -2157,12 +2162,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)
@@ -2195,12 +2201,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..d6289f2f8f 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/option.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, FE_UTILS_PORT_MIN,
+ FE_UTILS_PORT_MAX, &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_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index af29dd7651..0c1437c989 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -26,6 +26,7 @@
#include "common/file_perm.h"
#include "common/fe_memutils.h"
#include "common/logging.h"
+#include "fe_utils/option.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "libpq/pqsignal.h"
@@ -705,6 +706,10 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:",
long_options, &option_index)) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
/* general options */
@@ -712,12 +717,14 @@ main(int argc, char **argv)
outfile = pg_strdup(optarg);
break;
case 'F':
- fsync_interval = atoi(optarg) * 1000;
- if (fsync_interval < 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 0, INT_MAX / 1000, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("invalid fsync interval \"%s\"", optarg);
+ pg_log_error("invalid fsync interval: %s", parse_error);
exit(1);
}
+ fsync_interval = parsed * 1000;
break;
case 'n':
noloop = 1;
@@ -733,11 +740,14 @@ main(int argc, char **argv)
dbhost = pg_strdup(optarg);
break;
case 'p':
- if (atoi(optarg) <= 0)
+ s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN,
+ FE_UTILS_PORT_MAX, &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':
@@ -790,12 +800,14 @@ main(int argc, char **argv)
plugin = pg_strdup(optarg);
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);
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..ad03a0d080 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/option.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 f01fea5b91..265e88fbab 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/option.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..b01c169c14 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/option.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..8e8cffaed2 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/option.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, FE_UTILS_PORT_MIN,
+ FE_UTILS_PORT_MAX, &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, FE_UTILS_PORT_MIN,
+ FE_UTILS_PORT_MAX, &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/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 600f1deb71..7eb3a6ff63 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -36,6 +36,7 @@
#include "common/logging.h"
#include "common/string.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/option.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "portability/instr_time.h"
@@ -5094,6 +5095,9 @@ main(int argc, char **argv)
while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) !=
-1)
{
char *script;
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
switch (c)
{
@@ -5125,13 +5129,15 @@ main(int argc, char **argv)
break;
case 'c':
benchmarking_option_set = true;
- nclients = atoi(optarg);
- if (nclients <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid number of clients: \"%s\"\n",
- optarg);
+ fprintf(stderr, "invalid number of clients: %s\n",
+ parse_error);
exit(1);
}
+ nclients = parsed;
#ifdef HAVE_GETRLIMIT
#ifdef RLIMIT_NOFILE /* most platforms use RLIMIT_NOFILE */
if (getrlimit(RLIMIT_NOFILE, &rlim) == -1)
@@ -5153,13 +5159,15 @@ main(int argc, char **argv)
break;
case 'j': /* jobs */
benchmarking_option_set = true;
- nthreads = atoi(optarg);
- if (nthreads <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid number of threads: \"%s\"\n",
- optarg);
+ fprintf(stderr, "invalid number of threads: %s\n",
+ parse_error);
exit(1);
}
+ nthreads = parsed;
#ifndef ENABLE_THREAD_SAFETY
if (nthreads != 1)
{
@@ -5178,31 +5186,37 @@ main(int argc, char **argv)
break;
case 's':
scale_given = true;
- scale = atoi(optarg);
- if (scale <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg);
+ fprintf(stderr, "invalid scaling factor: %s\n", parse_error);
exit(1);
}
+ scale = parsed;
break;
case 't':
benchmarking_option_set = true;
- nxacts = atoi(optarg);
- if (nxacts <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid number of transactions: \"%s\"\n",
- optarg);
+ fprintf(stderr, "invalid number of transactions: %s\n",
+ parse_error);
exit(1);
}
+ nxacts = parsed;
break;
case 'T':
benchmarking_option_set = true;
- duration = atoi(optarg);
- if (duration <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
fprintf(stderr, "invalid duration: \"%s\"\n", optarg);
exit(1);
}
+ duration = parsed;
break;
case 'U':
login = pg_strdup(optarg);
@@ -5261,12 +5275,14 @@ main(int argc, char **argv)
break;
case 'F':
initialization_option_set = true;
- fillfactor = atoi(optarg);
- if (fillfactor < 10 || fillfactor > 100)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 10, 100, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg);
+ fprintf(stderr, "invalid fillfactor: %s\n", parse_error);
exit(1);
}
+ fillfactor = parsed;
break;
case 'M':
benchmarking_option_set = true;
@@ -5282,13 +5298,15 @@ main(int argc, char **argv)
break;
case 'P':
benchmarking_option_set = true;
- progress = atoi(optarg);
- if (progress <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid thread progress delay: \"%s\"\n",
- optarg);
+ fprintf(stderr, "invalid thread progress delay: %s\n",
+ parse_error);
exit(1);
}
+ progress = parsed;
break;
case 'R':
{
@@ -5343,13 +5361,15 @@ main(int argc, char **argv)
break;
case 5: /* aggregate-interval */
benchmarking_option_set = true;
- agg_interval = atoi(optarg);
- if (agg_interval <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n",
- optarg);
+ fprintf(stderr, "invalid number of seconds for aggregation: %s\n",
+ parse_error);
exit(1);
}
+ agg_interval = parsed;
break;
case 6: /* progress-timestamp */
progress_timestamp = true;
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index f00aec15de..5024aaad67 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -15,6 +15,7 @@
#include "common.h"
#include "common/logging.h"
#include "fe_utils/connect.h"
+#include "fe_utils/option.h"
#include "fe_utils/simple_list.h"
#include "fe_utils/string_utils.h"
#include "scripts_parallel.h"
@@ -105,6 +106,10 @@ main(int argc, char *argv[])
/* process command-line options */
while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'h':
@@ -147,12 +152,14 @@ main(int argc, char *argv[])
simple_string_list_append(&indexes, optarg);
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("number of parallel jobs must be at least 1");
+ pg_log_error("invalid number of parallel jobs: %s", parse_error);
exit(1);
}
+ concurrentCons = parsed;
break;
case 'v':
verbose = true;
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 2c7219239f..9266966d62 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -17,6 +17,7 @@
#include "common.h"
#include "common/logging.h"
#include "fe_utils/connect.h"
+#include "fe_utils/option.h"
#include "fe_utils/simple_list.h"
#include "fe_utils/string_utils.h"
#include "scripts_parallel.h"
@@ -124,6 +125,10 @@ main(int argc, char *argv[])
while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options, &optindex)) != -1)
{
+ pg_strtoint_status s;
+ int64 parsed;
+ char *parse_error;
+
switch (c)
{
case 'h':
@@ -175,12 +180,14 @@ main(int argc, char *argv[])
vacopts.verbose = true;
break;
case 'j':
- concurrentCons = atoi(optarg);
- if (concurrentCons <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("number of parallel jobs must be at least 1");
+ pg_log_error("invalid number of parallel jobs: %s", parse_error);
exit(1);
}
+ concurrentCons = parsed;
break;
case 2:
maintenance_db = pg_strdup(optarg);
@@ -195,20 +202,24 @@ main(int argc, char *argv[])
vacopts.skip_locked = true;
break;
case 6:
- vacopts.min_xid_age = atoi(optarg);
- if (vacopts.min_xid_age <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("minimum transaction ID age must be at least 1");
+ pg_log_error("invalid minimum transaction ID age: %s", parse_error);
exit(1);
}
+ vacopts.min_xid_age = parsed;
break;
case 7:
- vacopts.min_mxid_age = atoi(optarg);
- if (vacopts.min_mxid_age <= 0)
+ s = pg_strtoint64_range(optarg, &parsed,
+ 1, INT_MAX, &parse_error);
+ if (s != PG_STRTOINT_OK)
{
- pg_log_error("minimum multixact ID age must be at least 1");
+ pg_log_error("invalid minimum multixact ID age: %s", parse_error);
exit(1);
}
+ vacopts.min_mxid_age = parsed;
break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index f2e516a2aa..83063abdcd 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -19,8 +19,8 @@ include $(top_builddir)/src/Makefile.global
override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
-OBJS = conditional.o mbprint.o print.o psqlscan.o recovery_gen.o \
- simple_list.o string_utils.o
+OBJS = conditional.o mbprint.o option.o print.o psqlscan.o \
+ recovery_gen.o simple_list.o string_utils.o
all: libpgfeutils.a
diff --git a/src/fe_utils/option.c b/src/fe_utils/option.c
new file mode 100644
index 0000000000..3924166718
--- /dev/null
+++ b/src/fe_utils/option.c
@@ -0,0 +1,46 @@
+/*-------------------------------------------------------------------------
+ *
+ * option.c
+ * argument parsing helpers for frontend code
+ *
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ *
+ * IDENTIFICATION
+ * src/fe_utils/option.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres_fe.h"
+
+#include "fe_utils/option.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/option.h b/src/include/fe_utils/option.h
new file mode 100644
index 0000000000..5833161fe5
--- /dev/null
+++ b/src/include/fe_utils/option.h
@@ -0,0 +1,31 @@
+/*
+ * option.h
+ * argument parsing helpers for frontend code
+ *
+ * Copyright (c) 2019, PostgreSQL Global Development Group
+ *
+ * src/include/fe_utils/option.h
+ */
+#ifndef FE_OPTION_H
+#define FE_OPTION_H
+
+#include <limits.h>
+
+#include "common/string.h"
+
+#define FE_UTILS_PORT_MIN 1024
+#define FE_UTILS_PORT_MAX ((1 << 16) - 1)
+
+/*
+ * 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_OPTION_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 7a103e6140..a9a01d22b7 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -142,7 +142,7 @@ sub mkvcbuild
our @pgcommonbkndfiles = @pgcommonallfiles;
our @pgfeutilsfiles = qw(
- conditional.c mbprint.c print.c psqlscan.l psqlscan.c
+ conditional.c mbprint.c option.c print.c psqlscan.l psqlscan.c
simple_list.c string_utils.c recovery_gen.c);
$libpgport = $solution->AddProject('libpgport', 'lib', 'misc');
On Mon, 7 Oct 2019 at 13:21, Joe Nelson <joe@begriffs.com> wrote:
>
> Ashutosh Sharma wrote:
> > One suggestion: The start value for port number is set to 1, however
> > it seems like the port number that falls in the range of 1-1023 is
> > reserved and can't be used. So, is it possible to have the start value
> > as 1024 instead of 1 ?
>
> Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> so the range can be updated in one place for all utilities.
(I've only had a very quick look at this, and FWIW, here's my opinion)
It's not for this patch to decide what ports clients can connect to
PostgreSQL on. As far as I'm aware Windows has no restrictions on what
ports unprivileged users can listen on. I think we're likely going to
upset a handful of people if we block the client tools from connecting
to ports < 1024.
> > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > header file "limits.h" has not been included in postgres_fe.h or
> > option.h
>
> Oops. Added limits.h now in option.h. The Postgres build accidentally
> worked on my system without explicitly including this header because
> __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> pulled in math.h with an #if which pulled in limits.h.
>
> > Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > The wording is a bit strange. How about something like pg_standy:
> > > invalid argument to -k: %s
>
> I updated the error messages in pg_standby.
>
> > > > *error = psprintf(_("could not parse '%s' as integer"), str);
> > >
> > > ... except that they would rather be more explicit about what the
> > > problem is. "insufficient digits" or "extraneous character", etc.
This part seems over-complex to me. What's wrong with just returning a
bool and if the caller gets "false", then just have it emit the error,
such as:
"compression level must be between %d and %d"
I see Michael's patch is adding this new return type, but really, is
there a good reason why we need to do something special when the user
does not pass in an integer?
Current patch:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer
I propose:
$ pg_dump -Z blah
compression level must be an integer in range 0..9
This might save a few round trips, e.g the current patch will do:
$ pg_dump -Z blah
invalid compression level: could not parse 'blah' as integer
$ pg_dump -Z 12345
invalid compression level: 12345 is outside range 0..9
$ ...
Also:
+ case PG_STRTOINT_RANGE_ERROR:
+ *error = psprintf(_("%s is outside range "
+ INT64_FORMAT ".." INT64_FORMAT),
+ str, min, max);
The translation string here must be consistent over all platforms. I
think this will cause issues if the translation string uses %ld and
the platform requires %lld?
I think what this patch should be really aiming for is to simplify the
client command-line argument parsing and adding what benefits it can.
I don't think there's really a need to make anything more complex than
it is already here.
I think you should maybe aim for 2 patches here.
Patch 1: Add new function to validate range and return bool indicating
if the string is an integer within range. Set output argument to the
int value if it is valid. Modify all locations where we currently
validate the range of the input arg to use the new function.
Patch 2: Add additional validation where we don't currently do
anything. e.g pg_dump -j
We can then see if there's any merit in patch 2 of if it's adding more
complexity than is really needed.
I also think some compilers won't like:
+ compressLevel = parsed;
given that "parsed" is int64 and "compressLevel" is int, surely some
compilers will warn of possible truncation? An explicit cast to int
should fix those or you could consider just writing a version of the
function for int32 and int64 and directly passing in the variable to
be set.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Oct 7, 2019 at 10:40 AM David Rowley
<david.rowley@2ndquadrant.com> wrote:
>
> On Mon, 7 Oct 2019 at 13:21, Joe Nelson <joe@begriffs.com> wrote:
> >
> > Ashutosh Sharma wrote:
> > > One suggestion: The start value for port number is set to 1, however
> > > it seems like the port number that falls in the range of 1-1023 is
> > > reserved and can't be used. So, is it possible to have the start value
> > > as 1024 instead of 1 ?
> >
> > Good idea -- changed it. I also created macros FE_UTILS_PORT_{MIN,MAX}
> > so the range can be updated in one place for all utilities.
>
> (I've only had a very quick look at this, and FWIW, here's my opinion)
>
> It's not for this patch to decide what ports clients can connect to
> PostgreSQL on. As far as I'm aware Windows has no restrictions on what
> ports unprivileged users can listen on. I think we're likely going to
> upset a handful of people if we block the client tools from connecting
> to ports < 1024.
>
AFAIU from the information given in the wiki page -[1], the port
numbers in the range of 1-1023 are for the standard protocols and
services. And there is nowhere mentioned that it is only true for some
OS and not for others. But, having said that I've just verified it on
Linux so I'm not aware of the behaviour on Windows.
[1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers
> > > Further, I encountered one syntax error (INT_MAX undeclared) as the
> > > header file "limits.h" has not been included in postgres_fe.h or
> > > option.h
> >
> > Oops. Added limits.h now in option.h. The Postgres build accidentally
> > worked on my system without explicitly including this header because
> > __has_builtin(__builtin_isinf) is true for me so src/include/port.h
> > pulled in math.h with an #if which pulled in limits.h.
> >
> > > Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > > The wording is a bit strange. How about something like pg_standy:
> > > > invalid argument to -k: %s
> >
> > I updated the error messages in pg_standby.
> >
> > > > > *error = psprintf(_("could not parse '%s' as integer"), str);
> > > >
> > > > ... except that they would rather be more explicit about what the
> > > > problem is. "insufficient digits" or "extraneous character", etc.
>
> This part seems over-complex to me. What's wrong with just returning a
> bool and if the caller gets "false", then just have it emit the error,
> such as:
>
> "compression level must be between %d and %d"
>
> I see Michael's patch is adding this new return type, but really, is
> there a good reason why we need to do something special when the user
> does not pass in an integer?
>
> Current patch:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
>
> I propose:
> $ pg_dump -Z blah
> compression level must be an integer in range 0..9
>
> This might save a few round trips, e.g the current patch will do:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
> $ pg_dump -Z 12345
> invalid compression level: 12345 is outside range 0..9
> $ ...
>
> Also:
>
> + case PG_STRTOINT_RANGE_ERROR:
> + *error = psprintf(_("%s is outside range "
> + INT64_FORMAT ".." INT64_FORMAT),
> + str, min, max);
>
> The translation string here must be consistent over all platforms. I
> think this will cause issues if the translation string uses %ld and
> the platform requires %lld?
>
> I think what this patch should be really aiming for is to simplify the
> client command-line argument parsing and adding what benefits it can.
> I don't think there's really a need to make anything more complex than
> it is already here.
>
> I think you should maybe aim for 2 patches here.
>
> Patch 1: Add new function to validate range and return bool indicating
> if the string is an integer within range. Set output argument to the
> int value if it is valid. Modify all locations where we currently
> validate the range of the input arg to use the new function.
>
> Patch 2: Add additional validation where we don't currently do
> anything. e.g pg_dump -j
>
> We can then see if there's any merit in patch 2 of if it's adding more
> complexity than is really needed.
>
> I also think some compilers won't like:
>
> + compressLevel = parsed;
>
> given that "parsed" is int64 and "compressLevel" is int, surely some
> compilers will warn of possible truncation? An explicit cast to int
> should fix those or you could consider just writing a version of the
> function for int32 and int64 and directly passing in the variable to
> be set.
>
> --
> David Rowley http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
On Mon, 7 Oct 2019 at 18:27, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > AFAIU from the information given in the wiki page -[1], the port > numbers in the range of 1-1023 are for the standard protocols and > services. And there is nowhere mentioned that it is only true for some > OS and not for others. But, having said that I've just verified it on > Linux so I'm not aware of the behaviour on Windows. > > [1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers Here are the results of a quick test on a windows machine: L:\Projects\Postgres\install\bin>echo test > c:\windows\test.txt Access is denied. L:\Projects\Postgres\install\bin>cat ../data/postgresql.conf | grep "port = " port = 543 # (change requires restart) L:\Projects\Postgres\install\bin>psql -p 543 postgres psql (11.5) WARNING: Console code page (850) differs from Windows code page (1252) 8-bit characters might not work correctly. See psql reference page "Notes for Windows users" for details. Type "help" for help. postgres=# -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Oct 7, 2019 at 11:05 AM David Rowley <david.rowley@2ndquadrant.com> wrote: > > On Mon, 7 Oct 2019 at 18:27, Ashutosh Sharma <ashu.coek88@gmail.com> wrote: > > AFAIU from the information given in the wiki page -[1], the port > > numbers in the range of 1-1023 are for the standard protocols and > > services. And there is nowhere mentioned that it is only true for some > > OS and not for others. But, having said that I've just verified it on > > Linux so I'm not aware of the behaviour on Windows. > > > > [1] - https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers > > Here are the results of a quick test on a windows machine: > > L:\Projects\Postgres\install\bin>echo test > c:\windows\test.txt > Access is denied. > > L:\Projects\Postgres\install\bin>cat ../data/postgresql.conf | grep "port = " > port = 543 # (change requires restart) > > L:\Projects\Postgres\install\bin>psql -p 543 postgres > psql (11.5) > WARNING: Console code page (850) differs from Windows code page (1252) > 8-bit characters might not work correctly. See psql reference > page "Notes for Windows users" for details. > Type "help" for help. > > postgres=# > Oh then that means all the unused ports (be it dedicated for some particular protocol or service) can be used on Windows. I just tried using port number 21 and 443 for postgres on my old Windows setup and it worked. See below, C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\pg_ctl -D ..\data -c -w -l logfile -o " -p 21" start waiting for server to start.... done server started C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\psql -d postgres -p 21 psql (10.5) WARNING: Console code page (437) differs from Windows code page (1252) 8-bit characters might not work correctly. See psql reference page "Notes for Windows users" for details. Type "help" for help. postgres=# \q C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\pg_ctl -D ..\data -c -w -l logfile stop waiting for server to shut down.... done server stopped C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\pg_ctl -D ..\data -c -w -l logfile -o " -p 443" start waiting for server to start.... done server started C:\Users\ashu\git-clone-postgresql\postgresql\TMP\test\bin>.\psql -d postgres -p 443 psql (10.5) WARNING: Console code page (437) differs from Windows code page (1252) 8-bit characters might not work correctly. See psql reference page "Notes for Windows users" for details. Type "help" for help. This looks a weird behaviour to me. I think this is probably one reason why people don't prefer using Windows. Anyways, thanks David for putting that point, it was really helpful. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com
David Rowley wrote:
> It's not for this patch to decide what ports clients can connect to
> PostgreSQL on. As far as I'm aware Windows has no restrictions on what
> ports unprivileged users can listen on. I think we're likely going to
> upset a handful of people if we block the client tools from connecting
> to ports < 1024.
Makes sense. We can instead allow any port number and if it errors at
connection time then the user will find out at that point.
> This part seems over-complex to me. What's wrong with just returning a
> bool and if the caller gets "false", then just have it emit the error,
> such as:
>
> "compression level must be between %d and %d"
>
> I see Michael's patch is adding this new return type, but really, is
> there a good reason why we need to do something special when the user
> does not pass in an integer?
Displaying the range when given a non-integer input could be misleading.
For instance, suppose we put as little restriction on the port number
range as possible, enforcing only that it's positive, between 1 and
INT_MAX. If someone passes a non-integer value, they would get a
message like:
invalid port number: must be an integer in range 1..2147483647
Sure, the parsing code will accept such a big number, but we don't want
to *suggest* the number. Notice if the user had passed a well-formed
number for the port it's unlikely to be greater than INT_MAX, so they
wouldn't have to see this weird message.
Perhaps you weren't suggesting to remove the upper limit from port
checking, just to change the lower limit from 1024 back to 1. In that
case we could keep it capped at 65535 and the error message above would
be OK.
Other utilities do have command line args that are allowed the whole
non-negative (but signed) int range, and their error message would show
the big number. It's not misleading in that case, but a little
ostentatious.
> Current patch:
> $ pg_dump -Z blah
> invalid compression level: could not parse 'blah' as integer
>
> I propose:
> $ pg_dump -Z blah
> compression level must be an integer in range 0..9
>
> This might save a few round trips, e.g the current patch will do:
I do see your reasoning that we're teasing people with a puzzle they
have to solve with multiple invocations. On the other hand, passing a
non-number for the compression level is pretty strange, and perhaps
explicitly calling out the mistake might make someone look more
carefully at what they -- or their script -- is doing.
> The translation string here must be consistent over all platforms. I
> think this will cause issues if the translation string uses %ld and
> the platform requires %lld?
A very good and subtle point. I'll change it to %lld so that a single
format string will work everywhere.
> I think you should maybe aim for 2 patches here.
>
> Patch 1: ...
>
> Patch 2: Add additional validation where we don't currently do
> anything. e.g pg_dump -j
>
> We can then see if there's any merit in patch 2 of if it's adding more
> complexity than is really needed.
Are you saying that my current patch adds extra constraints for
pg_dump's -j argument, or that in the future we could do that? Because I
don't believe the current patch adds any appreciable complexity for that
particular argument, other than ensuring the value is positive, which
doesn't seem too contentious.
Maybe we can see whether we can reach consensus on the current
parse-and-check combo patch, and if discussion drags on much longer then
try to split it up?
> I also think some compilers won't like:
>
> + compressLevel = parsed;
>
> given that "parsed" is int64 and "compressLevel" is int, surely some
> compilers will warn of possible truncation? An explicit cast to int
> should fix those
Good point, I bet some compilers (justly) warn about truncation. We've
checked the range so I'll add an explicit cast.
> or you could consider just writing a version of the function for int32
> and int64 and directly passing in the variable to be set.
One complication is that the destination values are often int rather
than int32, and I don't know their width in general (probably 32, maybe
16, but *possibly* 64?). The pg_strtoint64_range() function with range
argument of INT_MAX is flexible enough to handle whatever situation we
encounter. Am I overthinking this part?
--
Joe Nelson https://begriffs.com
On Tue, 8 Oct 2019 at 19:46, Joe Nelson <joe@begriffs.com> wrote:
>
> David Rowley wrote:
> > The translation string here must be consistent over all platforms. I
> > think this will cause issues if the translation string uses %ld and
> > the platform requires %lld?
>
> A very good and subtle point. I'll change it to %lld so that a single
> format string will work everywhere.
The way to do this is to make a temp buffer and snprintf into that
buffer then use %s.
See basebackup.c where it does:
char buf[64];
snprintf(buf, sizeof(buf), INT64_FORMAT, total_checksum_failures);
ereport(WARNING,
(errmsg("%s total checksum verification failures", buf)));
as an example.
> > I think you should maybe aim for 2 patches here.
> >
> > Patch 1: ...
> >
> > Patch 2: Add additional validation where we don't currently do
> > anything. e.g pg_dump -j
> >
> > We can then see if there's any merit in patch 2 of if it's adding more
> > complexity than is really needed.
>
> Are you saying that my current patch adds extra constraints for
> pg_dump's -j argument, or that in the future we could do that? Because I
> don't believe the current patch adds any appreciable complexity for that
> particular argument, other than ensuring the value is positive, which
> doesn't seem too contentious.
> Maybe we can see whether we can reach consensus on the current
> parse-and-check combo patch, and if discussion drags on much longer then
> try to split it up?
I just think you're more likely to get a committer onside if you made
it so they didn't have to consider if throwing errors where we
previously didn't would be a bad thing. It's quite common to get core
infrastructure in first then followup with code that uses it. This
would be core infrastructure plus some less controversial usages of
it, then follow up with more. This was really just a suggestion. I
didn't dig into the patch in enough detail to decide on how many
places could raise an error that would have silently just have done
something unexpected beforehand.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
> On Tue, 8 Oct 2019 at 19:46, Joe Nelson <joe@begriffs.com> wrote:
>> David Rowley wrote:
>>> The translation string here must be consistent over all platforms. I
>>> think this will cause issues if the translation string uses %ld and
>>> the platform requires %lld?
>> A very good and subtle point. I'll change it to %lld so that a single
>> format string will work everywhere.
> The way to do this is to make a temp buffer and snprintf into that
> buffer then use %s.
We have done it that way in the past, but it was mainly because we
couldn't be sure that snprintf was on board with %lld. I think that
the new consensus is that forcing use of "long long" is a less messy
solution (unless you need to back-patch the code). See commit
6a1cd8b92 for recent precedent.
regards, tom lane
Here's v6 of the patch. [x] Rebase on 20961ceaf0 [x] Don't call exit(1) after pg_fatal() [x] Use Tom Lane's suggestion for %lld in _() string [x] Allow full unsigned 16-bit range for ports (don't disallow ports 0-1023) [x] Explicitly cast parsed values to smaller integers -- 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..c6405d8b94 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/option.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: invalid argument for -k keepfiles: %s\n", + progname, parse_error); exit(2); } + keepfiles = (int) 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: invalid argument for -r maxretries: %s\n", + progname, parse_error); exit(2); } + maxretries = (int) 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: invalid argument for -s sleeptime: %s\n", + progname, parse_error); exit(2); } + sleeptime = (int) 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: invalid argument for -w maxwaittime: %s\n", + progname, parse_error); exit(2); } + maxwaittime = (int) 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 55ef13926d..2c0fbbc721 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -32,6 +32,7 @@ #include "common/logging.h" #include "common/string.h" #include "fe_utils/recovery_gen.h" +#include "fe_utils/option.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -2073,6 +2074,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': @@ -2157,12 +2162,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 = (int) parsed; break; case 'c': if (pg_strcasecmp(optarg, "fast") == 0) @@ -2195,12 +2201,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 = (int) 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..f967c11c44 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/option.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, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &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 = (int) 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 = (int) parsed; break; /* action */ case 1: diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index af29dd7651..10103ebffc 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -26,6 +26,7 @@ #include "common/file_perm.h" #include "common/fe_memutils.h" #include "common/logging.h" +#include "fe_utils/option.h" #include "getopt_long.h" #include "libpq-fe.h" #include "libpq/pqsignal.h" @@ -705,6 +706,10 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:", long_options, &option_index)) != -1) { + pg_strtoint_status s; + int64 parsed; + char *parse_error; + switch (c) { /* general options */ @@ -712,12 +717,14 @@ main(int argc, char **argv) outfile = pg_strdup(optarg); break; case 'F': - fsync_interval = atoi(optarg) * 1000; - if (fsync_interval < 0) + s = pg_strtoint64_range(optarg, &parsed, + 0, INT_MAX / 1000, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("invalid fsync interval \"%s\"", optarg); + pg_log_error("invalid fsync interval: %s", parse_error); exit(1); } + fsync_interval = (int) parsed * 1000; break; case 'n': noloop = 1; @@ -733,11 +740,14 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) + s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &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': @@ -790,12 +800,14 @@ main(int argc, char **argv) plugin = pg_strdup(optarg); 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 = (int) parsed * 1000; break; case 'S': replication_slot = pg_strdup(optarg); 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 eac4e771ab..b5ba9a86d2 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/option.h" #include "getopt_long.h" #include "utils/pidfile.h" @@ -2298,6 +2299,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': @@ -2361,7 +2366,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 = (int) 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 f01fea5b91..1bbf280aa8 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/option.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 = (int) 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 = (int) 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 = (int) 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..db9ad10fe7 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/option.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 = (int) 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 da9e1da78d..fd31a7c5e9 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/option.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,11 @@ 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); + user_opts.jobs = (int) parsed; break; case 'k': @@ -168,13 +177,19 @@ 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) - pg_fatal("invalid old port number\n"); + s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) + pg_fatal("invalid old port number: %s\n", parse_error); + old_cluster.port = (unsigned short) parsed; break; case 'P': - if ((new_cluster.port = atoi(optarg)) <= 0) - pg_fatal("invalid new port number\n"); + s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) + pg_fatal("invalid new port number: %s\n", parse_error); + new_cluster.port = (unsigned short) parsed; break; case 'r': diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 25919885b1..a8fc36c25d 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -36,6 +36,7 @@ #include "common/logging.h" #include "common/string.h" #include "fe_utils/conditional.h" +#include "fe_utils/option.h" #include "getopt_long.h" #include "libpq-fe.h" #include "portability/instr_time.h" @@ -5327,6 +5328,9 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1) { char *script; + pg_strtoint_status s; + int64 parsed; + char *parse_error; switch (c) { @@ -5358,13 +5362,15 @@ main(int argc, char **argv) break; case 'c': benchmarking_option_set = true; - nclients = atoi(optarg); - if (nclients <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of clients: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of clients: %s\n", + parse_error); exit(1); } + nclients = (int) parsed; #ifdef HAVE_GETRLIMIT #ifdef RLIMIT_NOFILE /* most platforms use RLIMIT_NOFILE */ if (getrlimit(RLIMIT_NOFILE, &rlim) == -1) @@ -5386,13 +5392,15 @@ main(int argc, char **argv) break; case 'j': /* jobs */ benchmarking_option_set = true; - nthreads = atoi(optarg); - if (nthreads <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of threads: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of threads: %s\n", + parse_error); exit(1); } + nthreads = (int) parsed; #ifndef ENABLE_THREAD_SAFETY if (nthreads != 1) { @@ -5411,31 +5419,37 @@ main(int argc, char **argv) break; case 's': scale_given = true; - scale = atoi(optarg); - if (scale <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg); + fprintf(stderr, "invalid scaling factor: %s\n", parse_error); exit(1); } + scale = (int) parsed; break; case 't': benchmarking_option_set = true; - nxacts = atoi(optarg); - if (nxacts <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of transactions: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of transactions: %s\n", + parse_error); exit(1); } + nxacts = (int) parsed; break; case 'T': benchmarking_option_set = true; - duration = atoi(optarg); - if (duration <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid duration: \"%s\"\n", optarg); + fprintf(stderr, "invalid duration: %s\n", parse_error); exit(1); } + duration = (int) parsed; break; case 'U': login = pg_strdup(optarg); @@ -5494,12 +5508,14 @@ main(int argc, char **argv) break; case 'F': initialization_option_set = true; - fillfactor = atoi(optarg); - if (fillfactor < 10 || fillfactor > 100) + s = pg_strtoint64_range(optarg, &parsed, + 10, 100, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg); + fprintf(stderr, "invalid fillfactor: %s\n", parse_error); exit(1); } + fillfactor = (int) parsed; break; case 'M': benchmarking_option_set = true; @@ -5515,13 +5531,15 @@ main(int argc, char **argv) break; case 'P': benchmarking_option_set = true; - progress = atoi(optarg); - if (progress <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid thread progress delay: \"%s\"\n", - optarg); + fprintf(stderr, "invalid thread progress delay: %s\n", + parse_error); exit(1); } + progress = (int) parsed; break; case 'R': { @@ -5576,13 +5594,15 @@ main(int argc, char **argv) break; case 5: /* aggregate-interval */ benchmarking_option_set = true; - agg_interval = atoi(optarg); - if (agg_interval <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of seconds for aggregation: %s\n", + parse_error); exit(1); } + agg_interval = (int) parsed; break; case 6: /* progress-timestamp */ progress_timestamp = true; diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index f00aec15de..0dc5379bbf 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -15,6 +15,7 @@ #include "common.h" #include "common/logging.h" #include "fe_utils/connect.h" +#include "fe_utils/option.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" #include "scripts_parallel.h" @@ -105,6 +106,10 @@ main(int argc, char *argv[]) /* process command-line options */ while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1) { + pg_strtoint_status s; + int64 parsed; + char *parse_error; + switch (c) { case 'h': @@ -147,12 +152,14 @@ main(int argc, char *argv[]) simple_string_list_append(&indexes, optarg); break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("invalid number of parallel jobs: %s", parse_error); exit(1); } + concurrentCons = (int) parsed; break; case 'v': verbose = true; diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 2c7219239f..ada5dd2f02 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -17,6 +17,7 @@ #include "common.h" #include "common/logging.h" #include "fe_utils/connect.h" +#include "fe_utils/option.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" #include "scripts_parallel.h" @@ -124,6 +125,10 @@ main(int argc, char *argv[]) while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options, &optindex)) != -1) { + pg_strtoint_status s; + int64 parsed; + char *parse_error; + switch (c) { case 'h': @@ -175,12 +180,14 @@ main(int argc, char *argv[]) vacopts.verbose = true; break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("invalid number of parallel jobs: %s", parse_error); exit(1); } + concurrentCons = (int) parsed; break; case 2: maintenance_db = pg_strdup(optarg); @@ -195,20 +202,24 @@ main(int argc, char *argv[]) vacopts.skip_locked = true; break; case 6: - vacopts.min_xid_age = atoi(optarg); - if (vacopts.min_xid_age <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("minimum transaction ID age must be at least 1"); + pg_log_error("invalid minimum transaction ID age: %s", parse_error); exit(1); } + vacopts.min_xid_age = (int) parsed; break; case 7: - vacopts.min_mxid_age = atoi(optarg); - if (vacopts.min_mxid_age <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (s != PG_STRTOINT_OK) { - pg_log_error("minimum multixact ID age must be at least 1"); + pg_log_error("invalid minimum multixact ID age: %s", parse_error); exit(1); } + vacopts.min_mxid_age = (int) parsed; break; default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile index f2e516a2aa..83063abdcd 100644 --- a/src/fe_utils/Makefile +++ b/src/fe_utils/Makefile @@ -19,8 +19,8 @@ include $(top_builddir)/src/Makefile.global override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -OBJS = conditional.o mbprint.o print.o psqlscan.o recovery_gen.o \ - simple_list.o string_utils.o +OBJS = conditional.o mbprint.o option.o print.o psqlscan.o \ + recovery_gen.o simple_list.o string_utils.o all: libpgfeutils.a diff --git a/src/fe_utils/option.c b/src/fe_utils/option.c new file mode 100644 index 0000000000..477081d044 --- /dev/null +++ b/src/fe_utils/option.c @@ -0,0 +1,45 @@ +/*------------------------------------------------------------------------- + * + * option.c + * argument parsing helpers for frontend code + * + * Copyright (c) 2019, PostgreSQL Global Development Group + * + * + * IDENTIFICATION + * src/fe_utils/option.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres_fe.h" + +#include "fe_utils/option.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 %lld..%lld"), + str, (long long) min, (long long) max); + break; + default: + pg_unreachable(); + } + return s; +} diff --git a/src/include/fe_utils/option.h b/src/include/fe_utils/option.h new file mode 100644 index 0000000000..89511d30ef --- /dev/null +++ b/src/include/fe_utils/option.h @@ -0,0 +1,31 @@ +/* + * option.h + * argument parsing helpers for frontend code + * + * Copyright (c) 2019, PostgreSQL Global Development Group + * + * src/include/fe_utils/option.h + */ +#ifndef FE_OPTION_H +#define FE_OPTION_H + +#include <limits.h> + +#include "common/string.h" + +#define FE_UTILS_PORT_MIN 0 +#define FE_UTILS_PORT_MAX ((1 << 16) - 1) + +/* + * 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_OPTION_H */ diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index 7a103e6140..a9a01d22b7 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -142,7 +142,7 @@ sub mkvcbuild our @pgcommonbkndfiles = @pgcommonallfiles; our @pgfeutilsfiles = qw( - conditional.c mbprint.c print.c psqlscan.l psqlscan.c + conditional.c mbprint.c option.c print.c psqlscan.l psqlscan.c simple_list.c string_utils.c recovery_gen.c); $libpgport = $solution->AddProject('libpgport', 'lib', 'misc');
At Fri, 11 Oct 2019 23:27:54 -0500, Joe Nelson <joe@begriffs.com> wrote in > Here's v6 of the patch. > > [x] Rebase on 20961ceaf0 > [x] Don't call exit(1) after pg_fatal() > [x] Use Tom Lane's suggestion for %lld in _() string > [x] Allow full unsigned 16-bit range for ports (don't disallow ports 0-1023) > [x] Explicitly cast parsed values to smaller integers Thank you for the new version. By the way in the upthread, At Tue, 8 Oct 2019 01:46:51 -0500, Joe Nelson <joe@begriffs.com> wrote in > > I see Michael's patch is adding this new return type, but really, is > > there a good reason why we need to do something special when the user > > does not pass in an integer? I agree to David in that it's better to avoid that kind of complexity if possible. The significant point of separating them was that you don't want to suggest a false value range for non-integer inputs. Looking the latest patch, the wrong suggestions and the complexity introduced by the %lld alternative are already gone. So I think we're reaching the simple solution where pg_strtoint64_range doesn't need to be involved in message building. "<hoge> must be an integer in the range (mm .. xx)" Doesn't the generic message work for all kinds of failure here? # It is also easy for transators than the split message case. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro Horiguchi wrote: > Doesn't the generic message work for all kinds of failure here? Yeah it does now. I updated pg_strtoint64_range to generate the same message for all errors. > So I think we're reaching the simple solution where > pg_strtoint64_range doesn't need to be involved in message building. Even though there's only one message, it still seems best to have the function create the error string. That way the string stays consistent and isn't duplicated across the code. -- 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..47a5dcd675 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/option.h" const char *progname; @@ -678,6 +679,10 @@ main(int argc, char **argv) while ((c = getopt(argc, argv, "cdk:lr:s:t:w:")) != -1) { + bool 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) { - fprintf(stderr, "%s: -k keepfiles must be >= 0\n", progname); + fprintf(stderr, "%s: invalid argument for -k keepfiles: %s\n", + progname, parse_error); exit(2); } + keepfiles = (int) 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) { - fprintf(stderr, "%s: -r maxretries must be >= 0\n", progname); + fprintf(stderr, "%s: invalid argument for -r maxretries: %s\n", + progname, parse_error); exit(2); } + maxretries = (int) 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) { - fprintf(stderr, "%s: -s sleeptime incorrectly set\n", progname); + fprintf(stderr, "%s: invalid argument for -s sleeptime: %s\n", + progname, parse_error); exit(2); } + sleeptime = (int) 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) { - fprintf(stderr, "%s: -w maxwaittime incorrectly set\n", progname); + fprintf(stderr, "%s: invalid argument for -w maxwaittime: %s\n", + progname, parse_error); exit(2); } + maxwaittime = (int) 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 55ef13926d..438c189bc5 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -32,6 +32,7 @@ #include "common/logging.h" #include "common/string.h" #include "fe_utils/recovery_gen.h" +#include "fe_utils/option.h" #include "fe_utils/string_utils.h" #include "getopt_long.h" #include "libpq-fe.h" @@ -2073,6 +2074,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) { + bool s; + int64 parsed; + char *parse_error; + switch (c) { case 'C': @@ -2157,12 +2162,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_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("invalid compression level: %s", parse_error); exit(1); } + compresslevel = (int) parsed; break; case 'c': if (pg_strcasecmp(optarg, "fast") == 0) @@ -2195,12 +2201,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_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("invalid status interval: %s", parse_error); exit(1); } + standby_message_timeout = (int) 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..e5adc283e5 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/option.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) { + bool 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, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &parse_error); + if (!s) { - 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_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("invalid status interval: %s", parse_error); exit(1); } + standby_message_timeout = (int) 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_log_error("invalid compression level \"%s\"", optarg); + pg_log_error("invalid compression level: %s", parse_error); exit(1); } + compresslevel = (int) parsed; break; /* action */ case 1: diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index af29dd7651..99a4d64a69 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -26,6 +26,7 @@ #include "common/file_perm.h" #include "common/fe_memutils.h" #include "common/logging.h" +#include "fe_utils/option.h" #include "getopt_long.h" #include "libpq-fe.h" #include "libpq/pqsignal.h" @@ -705,6 +706,10 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "E:f:F:nvd:h:p:U:wWI:o:P:s:S:", long_options, &option_index)) != -1) { + bool s; + int64 parsed; + char *parse_error; + switch (c) { /* general options */ @@ -712,12 +717,14 @@ main(int argc, char **argv) outfile = pg_strdup(optarg); break; case 'F': - fsync_interval = atoi(optarg) * 1000; - if (fsync_interval < 0) + s = pg_strtoint64_range(optarg, &parsed, + 0, INT_MAX / 1000, &parse_error); + if (!s) { - pg_log_error("invalid fsync interval \"%s\"", optarg); + pg_log_error("invalid fsync interval: %s", parse_error); exit(1); } + fsync_interval = (int) parsed * 1000; break; case 'n': noloop = 1; @@ -733,11 +740,14 @@ main(int argc, char **argv) dbhost = pg_strdup(optarg); break; case 'p': - if (atoi(optarg) <= 0) + s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &parse_error); + if (!s) { - 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': @@ -790,12 +800,14 @@ main(int argc, char **argv) plugin = pg_strdup(optarg); 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_log_error("invalid status interval \"%s\"", optarg); + pg_log_error("invalid status interval: %s", parse_error); exit(1); } + standby_message_timeout = (int) parsed * 1000; break; case 'S': replication_slot = pg_strdup(optarg); 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 eac4e771ab..e734ffbd1f 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/option.h" #include "getopt_long.h" #include "utils/pidfile.h" @@ -2298,6 +2299,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) { + bool s; + int64 parsed; + char *parse_error; + switch (c) { case 'D': @@ -2361,7 +2366,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) + { + write_stderr(_("invalid timeout: %s\n"), parse_error); + exit(1); + } + wait_seconds = (int) 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 f01fea5b91..e4fc95ec3a 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/option.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) { + bool 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_log_error("invalid job count: %s", parse_error); + exit_nicely(1); + } + numWorkers = (int) 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_log_error("compression level must be in range 0..9"); + pg_log_error("invalid compression level: %s", parse_error); exit_nicely(1); } + compressLevel = (int) 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_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 = (int) 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..eef53d0db3 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/option.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) { + bool 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_log_error("invalid job count: %s", parse_error); + exit_nicely(1); + } + numWorkers = (int) 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 da9e1da78d..d0d506f486 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/option.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) { + bool s; + int64 parsed; + char *parse_error; + switch (option) { case 'b': @@ -129,7 +134,11 @@ 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_fatal("invalid job count: %s\n", parse_error); + user_opts.jobs = (int) parsed; break; case 'k': @@ -168,13 +177,19 @@ 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) - pg_fatal("invalid old port number\n"); + s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &parse_error); + if (!s) + pg_fatal("invalid old port number: %s\n", parse_error); + old_cluster.port = (unsigned short) parsed; break; case 'P': - if ((new_cluster.port = atoi(optarg)) <= 0) - pg_fatal("invalid new port number\n"); + s = pg_strtoint64_range(optarg, &parsed, FE_UTILS_PORT_MIN, + FE_UTILS_PORT_MAX, &parse_error); + if (!s) + pg_fatal("invalid new port number: %s\n", parse_error); + new_cluster.port = (unsigned short) parsed; break; case 'r': diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 25919885b1..c849080fe0 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -36,6 +36,7 @@ #include "common/logging.h" #include "common/string.h" #include "fe_utils/conditional.h" +#include "fe_utils/option.h" #include "getopt_long.h" #include "libpq-fe.h" #include "portability/instr_time.h" @@ -5327,6 +5328,9 @@ main(int argc, char **argv) while ((c = getopt_long(argc, argv, "iI:h:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1) { char *script; + bool s; + int64 parsed; + char *parse_error; switch (c) { @@ -5358,13 +5362,15 @@ main(int argc, char **argv) break; case 'c': benchmarking_option_set = true; - nclients = atoi(optarg); - if (nclients <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid number of clients: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of clients: %s\n", + parse_error); exit(1); } + nclients = (int) parsed; #ifdef HAVE_GETRLIMIT #ifdef RLIMIT_NOFILE /* most platforms use RLIMIT_NOFILE */ if (getrlimit(RLIMIT_NOFILE, &rlim) == -1) @@ -5386,13 +5392,15 @@ main(int argc, char **argv) break; case 'j': /* jobs */ benchmarking_option_set = true; - nthreads = atoi(optarg); - if (nthreads <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid number of threads: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of threads: %s\n", + parse_error); exit(1); } + nthreads = (int) parsed; #ifndef ENABLE_THREAD_SAFETY if (nthreads != 1) { @@ -5411,31 +5419,37 @@ main(int argc, char **argv) break; case 's': scale_given = true; - scale = atoi(optarg); - if (scale <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid scaling factor: \"%s\"\n", optarg); + fprintf(stderr, "invalid scaling factor: %s\n", parse_error); exit(1); } + scale = (int) parsed; break; case 't': benchmarking_option_set = true; - nxacts = atoi(optarg); - if (nxacts <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid number of transactions: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of transactions: %s\n", + parse_error); exit(1); } + nxacts = (int) parsed; break; case 'T': benchmarking_option_set = true; - duration = atoi(optarg); - if (duration <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid duration: \"%s\"\n", optarg); + fprintf(stderr, "invalid duration: %s\n", parse_error); exit(1); } + duration = (int) parsed; break; case 'U': login = pg_strdup(optarg); @@ -5494,12 +5508,14 @@ main(int argc, char **argv) break; case 'F': initialization_option_set = true; - fillfactor = atoi(optarg); - if (fillfactor < 10 || fillfactor > 100) + s = pg_strtoint64_range(optarg, &parsed, + 10, 100, &parse_error); + if (!s) { - fprintf(stderr, "invalid fillfactor: \"%s\"\n", optarg); + fprintf(stderr, "invalid fillfactor: %s\n", parse_error); exit(1); } + fillfactor = (int) parsed; break; case 'M': benchmarking_option_set = true; @@ -5515,13 +5531,15 @@ main(int argc, char **argv) break; case 'P': benchmarking_option_set = true; - progress = atoi(optarg); - if (progress <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid thread progress delay: \"%s\"\n", - optarg); + fprintf(stderr, "invalid thread progress delay: %s\n", + parse_error); exit(1); } + progress = (int) parsed; break; case 'R': { @@ -5576,13 +5594,15 @@ main(int argc, char **argv) break; case 5: /* aggregate-interval */ benchmarking_option_set = true; - agg_interval = atoi(optarg); - if (agg_interval <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - fprintf(stderr, "invalid number of seconds for aggregation: \"%s\"\n", - optarg); + fprintf(stderr, "invalid number of seconds for aggregation: %s\n", + parse_error); exit(1); } + agg_interval = (int) parsed; break; case 6: /* progress-timestamp */ progress_timestamp = true; diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index f00aec15de..0b75f3a1c2 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -15,6 +15,7 @@ #include "common.h" #include "common/logging.h" #include "fe_utils/connect.h" +#include "fe_utils/option.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" #include "scripts_parallel.h" @@ -105,6 +106,10 @@ main(int argc, char *argv[]) /* process command-line options */ while ((c = getopt_long(argc, argv, "h:p:U:wWeqS:d:ast:i:j:v", long_options, &optindex)) != -1) { + bool s; + int64 parsed; + char *parse_error; + switch (c) { case 'h': @@ -147,12 +152,14 @@ main(int argc, char *argv[]) simple_string_list_append(&indexes, optarg); break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("invalid number of parallel jobs: %s", parse_error); exit(1); } + concurrentCons = (int) parsed; break; case 'v': verbose = true; diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 2c7219239f..7e305e3746 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -17,6 +17,7 @@ #include "common.h" #include "common/logging.h" #include "fe_utils/connect.h" +#include "fe_utils/option.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" #include "scripts_parallel.h" @@ -124,6 +125,10 @@ main(int argc, char *argv[]) while ((c = getopt_long(argc, argv, "h:p:U:wWeqd:zZFat:fvj:", long_options, &optindex)) != -1) { + bool s; + int64 parsed; + char *parse_error; + switch (c) { case 'h': @@ -175,12 +180,14 @@ main(int argc, char *argv[]) vacopts.verbose = true; break; case 'j': - concurrentCons = atoi(optarg); - if (concurrentCons <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - pg_log_error("number of parallel jobs must be at least 1"); + pg_log_error("invalid number of parallel jobs: %s", parse_error); exit(1); } + concurrentCons = (int) parsed; break; case 2: maintenance_db = pg_strdup(optarg); @@ -195,20 +202,24 @@ main(int argc, char *argv[]) vacopts.skip_locked = true; break; case 6: - vacopts.min_xid_age = atoi(optarg); - if (vacopts.min_xid_age <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - pg_log_error("minimum transaction ID age must be at least 1"); + pg_log_error("invalid minimum transaction ID age: %s", parse_error); exit(1); } + vacopts.min_xid_age = (int) parsed; break; case 7: - vacopts.min_mxid_age = atoi(optarg); - if (vacopts.min_mxid_age <= 0) + s = pg_strtoint64_range(optarg, &parsed, + 1, INT_MAX, &parse_error); + if (!s) { - pg_log_error("minimum multixact ID age must be at least 1"); + pg_log_error("invalid minimum multixact ID age: %s", parse_error); exit(1); } + vacopts.min_mxid_age = (int) parsed; break; default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile index f2e516a2aa..83063abdcd 100644 --- a/src/fe_utils/Makefile +++ b/src/fe_utils/Makefile @@ -19,8 +19,8 @@ include $(top_builddir)/src/Makefile.global override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS) -OBJS = conditional.o mbprint.o print.o psqlscan.o recovery_gen.o \ - simple_list.o string_utils.o +OBJS = conditional.o mbprint.o option.o print.o psqlscan.o \ + recovery_gen.o simple_list.o string_utils.o all: libpgfeutils.a diff --git a/src/fe_utils/option.c b/src/fe_utils/option.c new file mode 100644 index 0000000000..5e42cc679b --- /dev/null +++ b/src/fe_utils/option.c @@ -0,0 +1,34 @@ +/*------------------------------------------------------------------------- + * + * option.c + * argument parsing helpers for frontend code + * + * Copyright (c) 2019, PostgreSQL Global Development Group + * + * + * IDENTIFICATION + * src/fe_utils/option.c + * + *------------------------------------------------------------------------- + */ + +#include "postgres_fe.h" + +#include "fe_utils/option.h" + +bool +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) + { + *error = psprintf(_("expecting integer in range %lld..%lld, " + "but got '%s'"), + (long long) min, (long long) max, str); + return false; + } + return true; +} diff --git a/src/include/fe_utils/option.h b/src/include/fe_utils/option.h new file mode 100644 index 0000000000..c79afe5b29 --- /dev/null +++ b/src/include/fe_utils/option.h @@ -0,0 +1,29 @@ +/* + * option.h + * argument parsing helpers for frontend code + * + * Copyright (c) 2019, PostgreSQL Global Development Group + * + * src/include/fe_utils/option.h + */ +#ifndef FE_OPTION_H +#define FE_OPTION_H + +#include <limits.h> + +#include "common/string.h" + +#define FE_UTILS_PORT_MIN 0 +#define FE_UTILS_PORT_MAX ((1 << 16) - 1) + +/* + * Parses string as int64 like pg_strtoint64, but fails + * 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. + */ +bool pg_strtoint64_range(const char *str, int64 *result, + int64 min, int64 max, char **error); + +#endif /* FE_OPTION_H */ diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index 7a103e6140..a9a01d22b7 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -142,7 +142,7 @@ sub mkvcbuild our @pgcommonbkndfiles = @pgcommonallfiles; our @pgfeutilsfiles = qw( - conditional.c mbprint.c print.c psqlscan.l psqlscan.c + conditional.c mbprint.c option.c print.c psqlscan.l psqlscan.c simple_list.c string_utils.c recovery_gen.c); $libpgport = $solution->AddProject('libpgport', 'lib', 'misc');
I see this patch has been moved to the next commitfest. What's the next step, does it need another review? -- Joe Nelson https://begriffs.com
On 2019-12-06 18:43, Joe Nelson wrote: > I see this patch has been moved to the next commitfest. What's the next > step, does it need another review? I think you need to promote your patch better. The thread subject and the commit fest entry title are somewhat nonsensical and no longer match what the patch actually does. The patch contains no commit message and no documentation or test changes, so it's not easy to make out what it's supposed to do or verify that it does so correctly. A reviewer would have to take this patch on faith or manually test every single command line option to see what it does. Moreover, a lot of this error message tweaking is opinion-based, so it's more burdensome to do an objective review. This patch is competing for attention against more than 200 other patches that have more going for them in these aspects. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Joe Nelson wrote: > > I see this patch has been moved to the next commitfest. What's the next > > step, does it need another review? Peter Eisentraut wrote: > I think you need to promote your patch better. Thanks for taking the time to revive this thread. Quick sales pitch for the patch: * Standardizes bounds checking and error message format in utilities pg_standby, pg_basebackup, pg_receivewal, pg_recvlogical, pg_ctl, pg_dump, pg_restore, pg_upgrade, pgbench, reindexdb, and vacuumdb * Removes Undefined Behavior caused by atoi() as described in the C99 standard, section 7.20.1 * Unifies integer parsing between the front- and back-end using functions provided in https://commitfest.postgresql.org/25/2272/ In reality I doubt my patch is fixing any pressing problem, it's just a small refactor. > The thread subject and the commit fest entry title are somewhat > nonsensical and no longer match what the patch actually does. I thought changing the subject line might be discouraged, but since you suggest doing it, I just did. Updated the title of the commitfest entry https://commitfest.postgresql.org/26/2197/ as well. > The patch contains no commit message Does this list not accept plain patches, compatible with git-apply? (Maybe your point is that I should make it as easy for committers as possible, and asking them to invent a commit message is just extra work.) > and no documentation or test changes The interfaces of the utilities remain the same. Same flags. The only change would be the error messages produced for incorrect values. The tests I ran passed successfully, but perhaps there were others I didn't try running and should have. > Moreover, a lot of this error message tweaking is opinion-based, so > it's more burdensome to do an objective review. This patch is > competing for attention against more than 200 other patches that have > more going for them in these aspects. True. I think the code looks nicer and the errors are more informative, but I'll leave it in the community's hands to determine if this is something they want. Once again, I appreciate your taking the time to help me with this process.
On 2019-Dec-06, Joe Nelson wrote: > I see this patch has been moved to the next commitfest. What's the next > step, does it need another review? This patch doesn't currently apply; it has conflicts with at least 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with fuzz. Please post an updated version so that it can move forward. On the other hand, I doubt that patching pg_standby is productive. I would just leave that out entirely. See this thread from 2014 https://postgr.es/m/545946E9.8060504@gmx.net -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 11 Feb 2020, at 17:54, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Dec-06, Joe Nelson wrote: > >> I see this patch has been moved to the next commitfest. What's the next >> step, does it need another review? > > This patch doesn't currently apply; it has conflicts with at least > 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with > fuzz. Please post an updated version so that it can move forward. Ping. With the 2020-03 CommitFest now under way, are you able to supply a rebased patch for consideration? cheers ./daniel
Daniel Gustafsson wrote: > > On 11 Feb 2020, at 17:54, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > This patch doesn't currently apply; it has conflicts with at least > > 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with > > fuzz. Please post an updated version so that it can move forward. > > Ping. With the 2020-03 CommitFest now under way, are you able to supply a > rebased patch for consideration? My patch relies on another that was previously returned with feedback in the 2019-11 CF: https://commitfest.postgresql.org/25/2272/ I've lost interest in continuing to rebase this. Someone else can take over the work if they are interested in it. Otherwise just close the CF entry.
> On 5 Mar 2020, at 06:06, Joe Nelson <joe@begriffs.com> wrote: > > Daniel Gustafsson wrote: > >>> On 11 Feb 2020, at 17:54, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >>> >>> This patch doesn't currently apply; it has conflicts with at least >>> 01368e5d9da7 and 7e735035f208; even in 7e735035f208^ it applies with >>> fuzz. Please post an updated version so that it can move forward. >> >> Ping. With the 2020-03 CommitFest now under way, are you able to supply a >> rebased patch for consideration? > > My patch relies on another that was previously returned with feedback in > the 2019-11 CF: https://commitfest.postgresql.org/25/2272/ > > I've lost interest in continuing to rebase this. Someone else can take over the > work if they are interested in it. Otherwise just close the CF entry. Ok, I'm marking this as withdrawn in the CF app, anyone interested can pick it up where this thread left off and re-submit. Thanks for working on it! cheers ./daniel