Skip to content

Commit 6f164e6

Browse files
michaelpqmichaelpq
committed
Unify parsing logic for command-line integer options
Most of the integer options for command-line binaries now make use of a single routine able to do the job, fixing issues with the detection of sloppy values caused for example by the use of atoi(), that fails on strings beginning with numerical characters with junk trailing characters. This commit cuts down the number of strings requiring translation by 26 per my count, switching the code to have two error types for invalid and out-of-range values instead. Much more could be done here, with float or even int64 options, but int32 was the most appealing case as it is possible to rely on strtol() to do the job reliably. Note that there are some exceptions for now, like pg_ctl or pg_upgrade that use their own logging logic. A couple of negative TAP tests required some adjustments for the new errors generated. pg_dump and pg_restore tracked the maximum number of parallel jobs within the option parsing. The code is refactored a bit to track that in the code dedicated to parallelism instead. Author: Kyotaro Horiguchi, Michael Paquier Reviewed-by: David Rowley, Álvaro Herrera Discussion: https://postgr.es/m/CALj2ACXqdG9WhqVoJ9zYf-iZt7sgK7Szv5USs=he6NnWQ2ofTA@mail.gmail.com
1 parent 6beb38c commit 6f164e6

File tree

17 files changed

+177
-199
lines changed

17 files changed

+177
-199
lines changed

‎src/bin/pg_amcheck/pg_amcheck.c

+3-5
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
*/
1313
#include "postgres_fe.h"
1414

15+
#include <limits.h>
1516
#include <time.h>
1617

1718
#include "catalog/pg_am_d.h"
@@ -326,12 +327,9 @@ main(int argc, char *argv[])
326327
append_btree_pattern(&opts.exclude, optarg, encoding);
327328
break;
328329
case 'j':
329-
opts.jobs = atoi(optarg);
330-
if (opts.jobs < 1)
331-
{
332-
pg_log_error("number of parallel jobs must be at least 1");
330+
if (!option_parse_int(optarg, "-j/--jobs", 1, INT_MAX,
331+
&opts.jobs))
333332
exit(1);
334-
}
335333
break;
336334
case 'p':
337335
port = pg_strdup(optarg);

‎src/bin/pg_basebackup/pg_basebackup.c

+7-10
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
#include "common/file_utils.h"
3232
#include "common/logging.h"
3333
#include "common/string.h"
34+
#include "fe_utils/option_utils.h"
3435
#include "fe_utils/recovery_gen.h"
3536
#include "fe_utils/string_utils.h"
3637
#include "getopt_long.h"
@@ -2371,12 +2372,9 @@ main(int argc, char **argv)
23712372
#endif
23722373
break;
23732374
case 'Z':
2374-
compresslevel = atoi(optarg);
2375-
if (compresslevel < 0 || compresslevel > 9)
2376-
{
2377-
pg_log_error("invalid compression level \"%s\"", optarg);
2375+
if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
2376+
&compresslevel))
23782377
exit(1);
2379-
}
23802378
break;
23812379
case 'c':
23822380
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2409,12 +2407,11 @@ main(int argc, char **argv)
24092407
dbgetpassword = 1;
24102408
break;
24112409
case 's':
2412-
standby_message_timeout = atoi(optarg) * 1000;
2413-
if (standby_message_timeout < 0)
2414-
{
2415-
pg_log_error("invalid status interval \"%s\"", optarg);
2410+
if (!option_parse_int(optarg, "-s/--status-interval", 0,
2411+
INT_MAX / 1000,
2412+
&standby_message_timeout))
24162413
exit(1);
2417-
}
2414+
standby_message_timeout *= 1000;
24182415
break;
24192416
case 'v':
24202417
verbose++;

‎src/bin/pg_basebackup/pg_receivewal.c

+8-15
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@
1515
#include "postgres_fe.h"
1616

1717
#include <dirent.h>
18+
#include <limits.h>
1819
#include <signal.h>
1920
#include <sys/stat.h>
2021
#include <unistd.h>
2122

2223
#include "access/xlog_internal.h"
2324
#include "common/file_perm.h"
2425
#include "common/logging.h"
26+
#include "fe_utils/option_utils.h"
2527
#include "getopt_long.h"
2628
#include "libpq-fe.h"
2729
#include "receivelog.h"
@@ -532,11 +534,6 @@ main(int argc, char **argv)
532534
dbhost = pg_strdup(optarg);
533535
break;
534536
case 'p':
535-
if (atoi(optarg) <= 0)
536-
{
537-
pg_log_error("invalid port number \"%s\"", optarg);
538-
exit(1);
539-
}
540537
dbport = pg_strdup(optarg);
541538
break;
542539
case 'U':
@@ -549,12 +546,11 @@ main(int argc, char **argv)
549546
dbgetpassword = 1;
550547
break;
551548
case 's':
552-
standby_message_timeout = atoi(optarg) * 1000;
553-
if (standby_message_timeout < 0)
554-
{
555-
pg_log_error("invalid status interval \"%s\"", optarg);
549+
if (!option_parse_int(optarg, "-s/--status-interval", 0,
550+
INT_MAX / 1000,
551+
&standby_message_timeout))
556552
exit(1);
557-
}
553+
standby_message_timeout *= 1000;
558554
break;
559555
case 'S':
560556
replication_slot = pg_strdup(optarg);
@@ -574,12 +570,9 @@ main(int argc, char **argv)
574570
verbose++;
575571
break;
576572
case 'Z':
577-
compresslevel = atoi(optarg);
578-
if (compresslevel < 0 || compresslevel > 9)
579-
{
580-
pg_log_error("invalid compression level \"%s\"", optarg);
573+
if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
574+
&compresslevel))
581575
exit(1);
582-
}
583576
break;
584577
/* action */
585578
case 1:

‎src/bin/pg_basebackup/pg_recvlogical.c

+10-15
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "postgres_fe.h"
1414

1515
#include <dirent.h>
16+
#include <limits.h>
1617
#include <sys/stat.h>
1718
#include <unistd.h>
1819
#ifdef HAVE_SYS_SELECT_H
@@ -23,6 +24,7 @@
2324
#include "common/fe_memutils.h"
2425
#include "common/file_perm.h"
2526
#include "common/logging.h"
27+
#include "fe_utils/option_utils.h"
2628
#include "getopt_long.h"
2729
#include "libpq-fe.h"
2830
#include "libpq/pqsignal.h"
@@ -739,12 +741,11 @@ main(int argc, char **argv)
739741
outfile = pg_strdup(optarg);
740742
break;
741743
case 'F':
742-
fsync_interval = atoi(optarg) * 1000;
743-
if (fsync_interval < 0)
744-
{
745-
pg_log_error("invalid fsync interval \"%s\"", optarg);
744+
if (!option_parse_int(optarg, "-F/--fsync-interval", 0,
745+
INT_MAX / 1000,
746+
&fsync_interval))
746747
exit(1);
747-
}
748+
fsync_interval *= 1000;
748749
break;
749750
case 'n':
750751
noloop = 1;
@@ -763,11 +764,6 @@ main(int argc, char **argv)
763764
dbhost = pg_strdup(optarg);
764765
break;
765766
case 'p':
766-
if (atoi(optarg) <= 0)
767-
{
768-
pg_log_error("invalid port number \"%s\"", optarg);
769-
exit(1);
770-
}
771767
dbport = pg_strdup(optarg);
772768
break;
773769
case 'U':
@@ -820,12 +816,11 @@ main(int argc, char **argv)
820816
plugin = pg_strdup(optarg);
821817
break;
822818
case 's':
823-
standby_message_timeout = atoi(optarg) * 1000;
824-
if (standby_message_timeout < 0)
825-
{
826-
pg_log_error("invalid status interval \"%s\"", optarg);
819+
if (!option_parse_int(optarg, "-s/--status-interval", 0,
820+
INT_MAX / 1000,
821+
&standby_message_timeout))
827822
exit(1);
828-
}
823+
standby_message_timeout *= 1000;
829824
break;
830825
case 'S':
831826
replication_slot = pg_strdup(optarg);

‎src/bin/pg_checksums/Makefile

+3
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ subdir = src/bin/pg_checksums
1515
top_builddir = ../../..
1616
include $(top_builddir)/src/Makefile.global
1717

18+
# We need libpq only because fe_utils does.
19+
LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_pgport)
20+
1821
OBJS = \
1922
$(WIN32RES) \
2023
pg_checksums.o

‎src/bin/pg_checksums/pg_checksums.c

+5-4
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "postgres_fe.h"
1616

1717
#include <dirent.h>
18+
#include <limits.h>
1819
#include <time.h>
1920
#include <sys/stat.h>
2021
#include <unistd.h>
@@ -24,6 +25,7 @@
2425
#include "common/file_perm.h"
2526
#include "common/file_utils.h"
2627
#include "common/logging.h"
28+
#include "fe_utils/option_utils.h"
2729
#include "getopt_long.h"
2830
#include "pg_getopt.h"
2931
#include "storage/bufpage.h"
@@ -518,11 +520,10 @@ main(int argc, char *argv[])
518520
mode = PG_MODE_ENABLE;
519521
break;
520522
case 'f':
521-
if (atoi(optarg) == 0)
522-
{
523-
pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
523+
if (!option_parse_int(optarg, "-f/--filenode", 0,
524+
INT_MAX,
525+
NULL))
524526
exit(1);
525-
}
526527
only_filenode = pstrdup(optarg);
527528
break;
528529
case 'N':

‎src/bin/pg_dump/parallel.h

+13
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,19 @@ typedef enum
3333
WFW_ALL_IDLE
3434
} WFW_WaitOption;
3535

36+
/*
37+
* Maximum number of parallel jobs allowed.
38+
*
39+
* On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
40+
* parallel jobs because that's the maximum limit for the
41+
* WaitForMultipleObjects() call.
42+
*/
43+
#ifdef WIN32
44+
#define PG_MAX_JOBS MAXIMUM_WAIT_OBJECTS
45+
#else
46+
#define PG_MAX_JOBS INT_MAX
47+
#endif
48+
3649
/* ParallelSlot is an opaque struct known only within parallel.c */
3750
typedef struct ParallelSlot ParallelSlot;
3851

‎src/bin/pg_dump/pg_dump.c

+11-36
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
#include "catalog/pg_type_d.h"
5757
#include "common/connect.h"
5858
#include "dumputils.h"
59+
#include "fe_utils/option_utils.h"
5960
#include "fe_utils/string_utils.h"
6061
#include "getopt_long.h"
6162
#include "libpq/libpq-fs.h"
@@ -322,14 +323,12 @@ main(int argc, char **argv)
322323
DumpableObject *boundaryObjs;
323324
int i;
324325
int optindex;
325-
char *endptr;
326326
RestoreOptions *ropt;
327327
Archive *fout; /* the script file */
328328
bool g_verbose = false;
329329
const char *dumpencoding = NULL;
330330
const char *dumpsnapshot = NULL;
331331
char *use_role = NULL;
332-
long rowsPerInsert;
333332
int numWorkers = 1;
334333
int compressLevel = -1;
335334
int plainText = 0;
@@ -487,7 +486,10 @@ main(int argc, char **argv)
487486
break;
488487

489488
case 'j': /* number of dump jobs */
490-
numWorkers = atoi(optarg);
489+
if (!option_parse_int(optarg, "-j/--jobs", 1,
490+
PG_MAX_JOBS,
491+
&numWorkers))
492+
exit_nicely(1);
491493
break;
492494

493495
case 'n': /* include schema(s) */
@@ -550,12 +552,9 @@ main(int argc, char **argv)
550552
break;
551553

552554
case 'Z': /* Compression Level */
553-
compressLevel = atoi(optarg);
554-
if (compressLevel < 0 || compressLevel > 9)
555-
{
556-
pg_log_error("compression level must be in range 0..9");
555+
if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
556+
&compressLevel))
557557
exit_nicely(1);
558-
}
559558
break;
560559

561560
case 0:
@@ -588,12 +587,9 @@ main(int argc, char **argv)
588587

589588
case 8:
590589
have_extra_float_digits = true;
591-
extra_float_digits = atoi(optarg);
592-
if (extra_float_digits < -15 || extra_float_digits > 3)
593-
{
594-
pg_log_error("extra_float_digits must be in range -15..3");
590+
if (!option_parse_int(optarg, "--extra-float-digits", -15, 3,
591+
&extra_float_digits))
595592
exit_nicely(1);
596-
}
597593
break;
598594

599595
case 9: /* inserts */
@@ -607,18 +603,9 @@ main(int argc, char **argv)
607603
break;
608604

609605
case 10: /* rows per insert */
610-
errno = 0;
611-
rowsPerInsert = strtol(optarg, &endptr, 10);
612-
613-
if (endptr == optarg || *endptr != '\0' ||
614-
rowsPerInsert <= 0 || rowsPerInsert > INT_MAX ||
615-
errno == ERANGE)
616-
{
617-
pg_log_error("rows-per-insert must be in range %d..%d",
618-
1, INT_MAX);
606+
if (!option_parse_int(optarg, "--rows-per-insert", 1, INT_MAX,
607+
&dopt.dump_inserts))
619608
exit_nicely(1);
620-
}
621-
dopt.dump_inserts = (int) rowsPerInsert;
622609
break;
623610

624611
case 11: /* include foreign data */
@@ -720,18 +707,6 @@ main(int argc, char **argv)
720707
if (!plainText)
721708
dopt.outputCreateDB = 1;
722709

723-
/*
724-
* On Windows we can only have at most MAXIMUM_WAIT_OBJECTS (= 64 usually)
725-
* parallel jobs because that's the maximum limit for the
726-
* WaitForMultipleObjects() call.
727-
*/
728-
if (numWorkers <= 0
729-
#ifdef WIN32
730-
|| numWorkers > MAXIMUM_WAIT_OBJECTS
731-
#endif
732-
)
733-
fatal("invalid number of parallel jobs");
734-
735710
/* Parallel backup only in the directory archive format so far */
736711
if (archiveFormat != archDirectory && numWorkers > 1)
737712
fatal("parallel backup only supported by the directory format");

‎src/bin/pg_dump/pg_restore.c

+5-17
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
#endif
4747

4848
#include "dumputils.h"
49+
#include "fe_utils/option_utils.h"
4950
#include "getopt_long.h"
5051
#include "parallel.h"
5152
#include "pg_backup_utils.h"
@@ -181,7 +182,10 @@ main(int argc, char **argv)
181182
break;
182183

183184
case 'j': /* number of restore jobs */
184-
numWorkers = atoi(optarg);
185+
if (!option_parse_int(optarg, "-j/--jobs", 1,
186+
PG_MAX_JOBS,
187+
&numWorkers))
188+
exit(1);
185189
break;
186190

187191
case 'l': /* Dump the TOC summary */
@@ -344,22 +348,6 @@ main(int argc, char **argv)
344348
exit_nicely(1);
345349
}
346350

347-
if (numWorkers <= 0)
348-
{
349-
pg_log_error("invalid number of parallel jobs");
350-
exit(1);
351-
}
352-
353-
/* See comments in pg_dump.c */
354-
#ifdef WIN32
355-
if (numWorkers > MAXIMUM_WAIT_OBJECTS)
356-
{
357-
pg_log_error("maximum number of parallel jobs is %d",
358-
MAXIMUM_WAIT_OBJECTS);
359-
exit(1);
360-
}
361-
#endif
362-
363351
/* Can't do single-txn mode with multiple connections */
364352
if (opts->single_txn && numWorkers > 1)
365353
{

0 commit comments

Comments
 (0)