Change atoi to strtol in same place

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
33 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Change atoi to strtol in same place

Surafel Temesgen
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


atoi-to-strtol-v1.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Tomas Vondra-4
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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
> 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@...

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':
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

David Rowley-3
On Wed, 24 Jul 2019 at 16:02, Joe Nelson <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Andres Freund
On 2019-07-24 16:57:42 +1200, David Rowley wrote:

> On Wed, 24 Jul 2019 at 16:02, Joe Nelson <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Michael Paquier-2
In reply to this post by David Rowley-3
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Álvaro Herrera
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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
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.


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
In reply to this post by Álvaro Herrera
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 */
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Robert Haas
On Tue, Sep 10, 2019 at 1:36 AM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Michael Paquier-2
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@...
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
In reply to this post by Michael Paquier-2
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
<[hidden email]>.

> 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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
In reply to this post by Michael Paquier-2
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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Robert Haas
In reply to this post by Michael Paquier-2
On Tue, Sep 10, 2019 at 11:54 PM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Alvaro Herrera-9
... 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


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
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.


Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Michael Paquier-2
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@...
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Joe Nelson
In reply to this post by Alvaro Herrera-9
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');
Reply | Threaded
Open this post in threaded view
|

Re: Change atoi to strtol in same place

Kyotaro Horiguchi-4
Hello.

At Sun, 29 Sep 2019 23:51:23 -0500, Joe Nelson <[hidden email]> wrote in <[hidden email]>

> 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


12