Thread
-
getopt() and strdup()
Bruce Momjian <bruce@momjian.us> — 2012-10-08T18:40:26Z
A while ago I noticed that in some places we strdup/pg_strdup() optarg strings from getopt(), and in some places we don't. If we needed the strdup(), the missing cases should generate errors. If we don't need them, the strdup() is unnecessary, and research confirms they are unnecessary. Should we remove the extra strdup/pg_strdup() calls, for consistency. I think we might have had old platforms that required it, but none are still supported today. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
-
Re: getopt() and strdup()
Andrew Dunstan <andrew@dunslane.net> — 2012-10-08T19:53:40Z
On 10/08/2012 02:40 PM, Bruce Momjian wrote: > A while ago I noticed that in some places we strdup/pg_strdup() optarg > strings from getopt(), and in some places we don't. > > If we needed the strdup(), the missing cases should generate errors. If > we don't need them, the strdup() is unnecessary, and research confirms > they are unnecessary. Should we remove the extra strdup/pg_strdup() > calls, for consistency. > > I think we might have had old platforms that required it, but none are > still supported today. ISTR there was a requirement at least on some platforms to use strdup/pg_strdup if the individual argument could be deformed, or the argument vector could be deformed. But maybe my memory is astray. I'm all for consistency, but only if we're darn sure it's not going to break things. cheers andrew
-
Re: getopt() and strdup()
Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-08T20:33:29Z
Bruce Momjian <bruce@momjian.us> writes: > A while ago I noticed that in some places we strdup/pg_strdup() optarg > strings from getopt(), and in some places we don't. > If we needed the strdup(), the missing cases should generate errors. If > we don't need them, the strdup() is unnecessary, and research confirms > they are unnecessary. Should we remove the extra strdup/pg_strdup() > calls, for consistency. What research? Given the number of different ways argv[] is handled on different platforms (cf ps_status.c), I am very unwilling to trust that it's safe to hang onto an argv string for long without strdup'ing it. > I think we might have had old platforms that required it, but none are > still supported today. And what's your grounds for stating that? All the alternatives in ps_status.c are still live code AFAICS. My feeling is it's more likely to be a good idea to be adding strdup's than removing them. regards, tom lane
-
Re: getopt() and strdup()
Bruce Momjian <bruce@momjian.us> — 2012-10-09T01:03:37Z
On Mon, Oct 8, 2012 at 04:33:29PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > A while ago I noticed that in some places we strdup/pg_strdup() optarg > > strings from getopt(), and in some places we don't. > > > If we needed the strdup(), the missing cases should generate errors. If > > we don't need them, the strdup() is unnecessary, and research confirms > > they are unnecessary. Should we remove the extra strdup/pg_strdup() > > calls, for consistency. > > What research? Given the number of different ways argv[] is handled > on different platforms (cf ps_status.c), I am very unwilling to trust > that it's safe to hang onto an argv string for long without strdup'ing > it. > > > I think we might have had old platforms that required it, but none are > > still supported today. > > And what's your grounds for stating that? All the alternatives in > ps_status.c are still live code AFAICS. > > My feeling is it's more likely to be a good idea to be adding strdup's > than removing them. Well, what we have now is either wrong or over-kill --- I don't know for sure which. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
-
Re: getopt() and strdup()
Bruce Momjian <bruce@momjian.us> — 2012-10-10T23:54:15Z
On Mon, Oct 8, 2012 at 09:03:37PM -0400, Bruce Momjian wrote: > On Mon, Oct 8, 2012 at 04:33:29PM -0400, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > A while ago I noticed that in some places we strdup/pg_strdup() optarg > > > strings from getopt(), and in some places we don't. > > > > > If we needed the strdup(), the missing cases should generate errors. If > > > we don't need them, the strdup() is unnecessary, and research confirms > > > they are unnecessary. Should we remove the extra strdup/pg_strdup() > > > calls, for consistency. > > > > What research? Given the number of different ways argv[] is handled > > on different platforms (cf ps_status.c), I am very unwilling to trust > > that it's safe to hang onto an argv string for long without strdup'ing > > it. > > > > > I think we might have had old platforms that required it, but none are > > > still supported today. > > > > And what's your grounds for stating that? All the alternatives in > > ps_status.c are still live code AFAICS. > > > > My feeling is it's more likely to be a good idea to be adding strdup's > > than removing them. > > Well, what we have now is either wrong or over-kill --- I don't know for > sure which. OK, I have developed the attached patch to add strdup/pg_strdup() calls to all saving of getopt optarg arguments. Also, do we want to centralize the definition of pg_strdup() in /port, or leave each module to define it on its own? I see pg_strdup() defined in these modules: /pgtop/contrib/oid2name /pgtop/contrib/pgbench /pgtop/contrib/pg_upgrade /pgtop/src/bin/initdb /pgtop/src/bin/pg_basebackup /pgtop/src/bin/pg_ctl /pgtop/src/bin/pg_dump /pgtop/src/bin/psql /pgtop/src/bin/scripts -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
-
Re: getopt() and strdup()
Bruce Momjian <bruce@momjian.us> — 2012-10-12T17:35:48Z
Applied. --------------------------------------------------------------------------- On Wed, Oct 10, 2012 at 07:54:15PM -0400, Bruce Momjian wrote: > On Mon, Oct 8, 2012 at 09:03:37PM -0400, Bruce Momjian wrote: > > On Mon, Oct 8, 2012 at 04:33:29PM -0400, Tom Lane wrote: > > > Bruce Momjian <bruce@momjian.us> writes: > > > > A while ago I noticed that in some places we strdup/pg_strdup() optarg > > > > strings from getopt(), and in some places we don't. > > > > > > > If we needed the strdup(), the missing cases should generate errors. If > > > > we don't need them, the strdup() is unnecessary, and research confirms > > > > they are unnecessary. Should we remove the extra strdup/pg_strdup() > > > > calls, for consistency. > > > > > > What research? Given the number of different ways argv[] is handled > > > on different platforms (cf ps_status.c), I am very unwilling to trust > > > that it's safe to hang onto an argv string for long without strdup'ing > > > it. > > > > > > > I think we might have had old platforms that required it, but none are > > > > still supported today. > > > > > > And what's your grounds for stating that? All the alternatives in > > > ps_status.c are still live code AFAICS. > > > > > > My feeling is it's more likely to be a good idea to be adding strdup's > > > than removing them. > > > > Well, what we have now is either wrong or over-kill --- I don't know for > > sure which. > > OK, I have developed the attached patch to add strdup/pg_strdup() calls > to all saving of getopt optarg arguments. > > Also, do we want to centralize the definition of pg_strdup() in /port, > or leave each module to define it on its own? I see pg_strdup() defined > in these modules: > > /pgtop/contrib/oid2name > /pgtop/contrib/pgbench > /pgtop/contrib/pg_upgrade > /pgtop/src/bin/initdb > /pgtop/src/bin/pg_basebackup > /pgtop/src/bin/pg_ctl > /pgtop/src/bin/pg_dump > /pgtop/src/bin/psql > /pgtop/src/bin/scripts > > -- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://enterprisedb.com > > + It's impossible for everything to be true. + > diff --git a/contrib/pg_archivecleanup/pg_archivecleanup.c b/contrib/pg_archivecleanup/pg_archivecleanup.c > new file mode 100644 > index 8f77998..e97a11c > *** a/contrib/pg_archivecleanup/pg_archivecleanup.c > --- b/contrib/pg_archivecleanup/pg_archivecleanup.c > *************** main(int argc, char **argv) > *** 299,305 **** > dryrun = true; > break; > case 'x': > ! additional_ext = optarg; /* Extension to remove from > * xlogfile names */ > break; > default: > --- 299,305 ---- > dryrun = true; > break; > case 'x': > ! additional_ext = strdup(optarg); /* Extension to remove from > * xlogfile names */ > break; > default: > diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c > new file mode 100644 > index 84941ed..659bd50 > *** a/contrib/pg_standby/pg_standby.c > --- b/contrib/pg_standby/pg_standby.c > *************** main(int argc, char **argv) > *** 643,649 **** > } > break; > case 't': /* Trigger file */ > ! triggerPath = optarg; > break; > case 'w': /* Max wait time */ > maxwaittime = atoi(optarg); > --- 643,649 ---- > } > break; > case 't': /* Trigger file */ > ! triggerPath = strdup(optarg); > break; > case 'w': /* Max wait time */ > maxwaittime = atoi(optarg); > diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c > new file mode 100644 > index c399d59..5d48aee > *** a/contrib/pgbench/pgbench.c > --- b/contrib/pgbench/pgbench.c > *************** main(int argc, char **argv) > *** 1995,2001 **** > is_init_mode++; > break; > case 'h': > ! pghost = optarg; > break; > case 'n': > is_no_vacuum++; > --- 1995,2001 ---- > is_init_mode++; > break; > case 'h': > ! pghost = pg_strdup(optarg); > break; > case 'n': > is_no_vacuum++; > *************** main(int argc, char **argv) > *** 2004,2010 **** > do_vacuum_accounts++; > break; > case 'p': > ! pgport = optarg; > break; > case 'd': > debug++; > --- 2004,2010 ---- > do_vacuum_accounts++; > break; > case 'p': > ! pgport = pg_strdup(optarg); > break; > case 'd': > debug++; > *************** main(int argc, char **argv) > *** 2090,2103 **** > } > break; > case 'U': > ! login = optarg; > break; > case 'l': > use_log = true; > break; > case 'f': > ttype = 3; > ! filename = optarg; > if (process_file(filename) == false || *sql_files[num_files - 1] == NULL) > exit(1); > break; > --- 2090,2103 ---- > } > break; > case 'U': > ! login = pg_strdup(optarg); > break; > case 'l': > use_log = true; > break; > case 'f': > ttype = 3; > ! filename = pg_strdup(optarg); > if (process_file(filename) == false || *sql_files[num_files - 1] == NULL) > exit(1); > break; > *************** main(int argc, char **argv) > *** 2143,2152 **** > /* This covers long options which take no argument. */ > break; > case 2: /* tablespace */ > ! tablespace = optarg; > break; > case 3: /* index-tablespace */ > ! index_tablespace = optarg; > break; > case 4: > sample_rate = atof(optarg); > --- 2143,2152 ---- > /* This covers long options which take no argument. */ > break; > case 2: /* tablespace */ > ! tablespace = pg_strdup(optarg); > break; > case 3: /* index-tablespace */ > ! index_tablespace = pg_strdup(optarg); > break; > case 4: > sample_rate = atof(optarg); > diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c > new file mode 100644 > index 34ddebb..11086e2 > *** a/src/backend/bootstrap/bootstrap.c > --- b/src/backend/bootstrap/bootstrap.c > *************** AuxiliaryProcessMain(int argc, char *arg > *** 241,247 **** > SetConfigOption("shared_buffers", optarg, PGC_POSTMASTER, PGC_S_ARGV); > break; > case 'D': > ! userDoption = optarg; > break; > case 'd': > { > --- 241,247 ---- > SetConfigOption("shared_buffers", optarg, PGC_POSTMASTER, PGC_S_ARGV); > break; > case 'D': > ! userDoption = strdup(optarg); > break; > case 'd': > { > diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c > new file mode 100644 > index e73caa8..dfe4049 > *** a/src/backend/postmaster/postmaster.c > --- b/src/backend/postmaster/postmaster.c > *************** PostmasterMain(int argc, char *argv[]) > *** 570,580 **** > break; > > case 'C': > ! output_config_variable = optarg; > break; > > case 'D': > ! userDoption = optarg; > break; > > case 'd': > --- 570,580 ---- > break; > > case 'C': > ! output_config_variable = strdup(optarg); > break; > > case 'D': > ! userDoption = strdup(optarg); > break; > > case 'd': > diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c > new file mode 100644 > index 9920d96..dd2019a > *** a/src/bin/pg_dump/pg_dump.c > --- b/src/bin/pg_dump/pg_dump.c > *************** main(int argc, char **argv) > *** 409,427 **** > break; > > case 'E': /* Dump encoding */ > ! dumpencoding = optarg; > break; > > case 'f': > ! filename = optarg; > break; > > case 'F': > ! format = optarg; > break; > > case 'h': /* server host */ > ! pghost = optarg; > break; > > case 'i': > --- 409,427 ---- > break; > > case 'E': /* Dump encoding */ > ! dumpencoding = pg_strdup(optarg); > break; > > case 'f': > ! filename = pg_strdup(optarg); > break; > > case 'F': > ! format = pg_strdup(optarg); > break; > > case 'h': /* server host */ > ! pghost = pg_strdup(optarg); > break; > > case 'i': > *************** main(int argc, char **argv) > *** 446,452 **** > break; > > case 'p': /* server port */ > ! pgport = optarg; > break; > > case 'R': > --- 446,452 ---- > break; > > case 'p': /* server port */ > ! pgport = pg_strdup(optarg); > break; > > case 'R': > *************** main(int argc, char **argv) > *** 471,477 **** > break; > > case 'U': > ! username = optarg; > break; > > case 'v': /* verbose */ > --- 471,477 ---- > break; > > case 'U': > ! username = pg_strdup(optarg); > break; > > case 'v': /* verbose */ > *************** main(int argc, char **argv) > *** 499,509 **** > break; > > case 2: /* lock-wait-timeout */ > ! lockWaitTimeout = optarg; > break; > > case 3: /* SET ROLE */ > ! use_role = optarg; > break; > > case 4: /* exclude table(s) data */ > --- 499,509 ---- > break; > > case 2: /* lock-wait-timeout */ > ! lockWaitTimeout = pg_strdup(optarg); > break; > > case 3: /* SET ROLE */ > ! use_role = pg_strdup(optarg); > break; > > case 4: /* exclude table(s) data */ > diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c > new file mode 100644 > index 10ce222..ca95bad > *** a/src/bin/pg_dump/pg_dumpall.c > --- b/src/bin/pg_dump/pg_dumpall.c > *************** main(int argc, char *argv[]) > *** 200,206 **** > break; > > case 'f': > ! filename = optarg; > appendPQExpBuffer(pgdumpopts, " -f "); > doShellQuoting(pgdumpopts, filename); > break; > --- 200,206 ---- > break; > > case 'f': > ! filename = pg_strdup(optarg); > appendPQExpBuffer(pgdumpopts, " -f "); > doShellQuoting(pgdumpopts, filename); > break; > *************** main(int argc, char *argv[]) > *** 210,216 **** > break; > > case 'h': > ! pghost = optarg; > appendPQExpBuffer(pgdumpopts, " -h "); > doShellQuoting(pgdumpopts, pghost); > break; > --- 210,216 ---- > break; > > case 'h': > ! pghost = pg_strdup(optarg); > appendPQExpBuffer(pgdumpopts, " -h "); > doShellQuoting(pgdumpopts, pghost); > break; > *************** main(int argc, char *argv[]) > *** 220,226 **** > break; > > case 'l': > ! pgdb = optarg; > break; > > case 'o': > --- 220,226 ---- > break; > > case 'l': > ! pgdb = pg_strdup(optarg); > break; > > case 'o': > *************** main(int argc, char *argv[]) > *** 232,238 **** > break; > > case 'p': > ! pgport = optarg; > appendPQExpBuffer(pgdumpopts, " -p "); > doShellQuoting(pgdumpopts, pgport); > break; > --- 232,238 ---- > break; > > case 'p': > ! pgport = pg_strdup(optarg); > appendPQExpBuffer(pgdumpopts, " -p "); > doShellQuoting(pgdumpopts, pgport); > break; > *************** main(int argc, char *argv[]) > *** 255,261 **** > break; > > case 'U': > ! pguser = optarg; > appendPQExpBuffer(pgdumpopts, " -U "); > doShellQuoting(pgdumpopts, pguser); > break; > --- 255,261 ---- > break; > > case 'U': > ! pguser = pg_strdup(optarg); > appendPQExpBuffer(pgdumpopts, " -U "); > doShellQuoting(pgdumpopts, pguser); > break; > *************** main(int argc, char *argv[]) > *** 289,295 **** > break; > > case 3: > ! use_role = optarg; > appendPQExpBuffer(pgdumpopts, " --role "); > doShellQuoting(pgdumpopts, use_role); > break; > --- 289,295 ---- > break; > > case 3: > ! use_role = pg_strdup(optarg); > appendPQExpBuffer(pgdumpopts, " --role "); > doShellQuoting(pgdumpopts, use_role); > break; > diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c > new file mode 100644 > index f6c835b..49d799b > *** a/src/bin/pg_dump/pg_restore.c > --- b/src/bin/pg_dump/pg_restore.c > *************** main(int argc, char **argv) > *** 238,244 **** > break; > > case 'U': > ! opts->username = optarg; > break; > > case 'v': /* verbose */ > --- 238,244 ---- > break; > > case 'U': > ! opts->username = pg_strdup(optarg); > break; > > case 'v': /* verbose */ > *************** main(int argc, char **argv) > *** 270,276 **** > break; > > case 2: /* SET ROLE */ > ! opts->use_role = optarg; > break; > > case 3: /* section */ > --- 270,276 ---- > break; > > case 2: /* SET ROLE */ > ! opts->use_role = pg_strdup(optarg); > break; > > case 3: /* section */ > diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c > new file mode 100644 > index 3fb12c9..1fcc47f > *** a/src/bin/psql/startup.c > --- b/src/bin/psql/startup.c > *************** parse_psql_options(int argc, char *argv[ > *** 411,417 **** > pset.popt.topt.format = PRINT_UNALIGNED; > break; > case 'c': > ! options->action_string = optarg; > if (optarg[0] == '\\') > { > options->action = ACT_SINGLE_SLASH; > --- 411,417 ---- > pset.popt.topt.format = PRINT_UNALIGNED; > break; > case 'c': > ! options->action_string = pg_strdup(optarg); > if (optarg[0] == '\\') > { > options->action = ACT_SINGLE_SLASH; > *************** parse_psql_options(int argc, char *argv[ > *** 421,427 **** > options->action = ACT_SINGLE_QUERY; > break; > case 'd': > ! options->dbname = optarg; > break; > case 'e': > SetVariable(pset.vars, "ECHO", "queries"); > --- 421,427 ---- > options->action = ACT_SINGLE_QUERY; > break; > case 'd': > ! options->dbname = pg_strdup(optarg); > break; > case 'e': > SetVariable(pset.vars, "ECHO", "queries"); > *************** parse_psql_options(int argc, char *argv[ > *** 431,444 **** > break; > case 'f': > options->action = ACT_FILE; > ! options->action_string = optarg; > break; > case 'F': > pset.popt.topt.fieldSep.separator = pg_strdup(optarg); > pset.popt.topt.fieldSep.separator_zero = false; > break; > case 'h': > ! options->host = optarg; > break; > case 'H': > pset.popt.topt.format = PRINT_HTML; > --- 431,444 ---- > break; > case 'f': > options->action = ACT_FILE; > ! options->action_string = pg_strdup(optarg); > break; > case 'F': > pset.popt.topt.fieldSep.separator = pg_strdup(optarg); > pset.popt.topt.fieldSep.separator_zero = false; > break; > case 'h': > ! options->host = pg_strdup(optarg); > break; > case 'H': > pset.popt.topt.format = PRINT_HTML; > *************** parse_psql_options(int argc, char *argv[ > *** 447,453 **** > options->action = ACT_LIST_DB; > break; > case 'L': > ! options->logfilename = optarg; > break; > case 'n': > options->no_readline = true; > --- 447,453 ---- > options->action = ACT_LIST_DB; > break; > case 'L': > ! options->logfilename = pg_strdup(optarg); > break; > case 'n': > options->no_readline = true; > *************** parse_psql_options(int argc, char *argv[ > *** 456,462 **** > setQFout(optarg); > break; > case 'p': > ! options->port = optarg; > break; > case 'P': > { > --- 456,462 ---- > setQFout(optarg); > break; > case 'p': > ! options->port = pg_strdup(optarg); > break; > case 'P': > { > *************** parse_psql_options(int argc, char *argv[ > *** 503,509 **** > pset.popt.topt.tableAttr = pg_strdup(optarg); > break; > case 'U': > ! options->username = optarg; > break; > case 'v': > { > --- 503,509 ---- > pset.popt.topt.tableAttr = pg_strdup(optarg); > break; > case 'U': > ! options->username = pg_strdup(optarg); > break; > case 'v': > { > diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c > new file mode 100644 > index b8ac675..261b438 > *** a/src/bin/scripts/clusterdb.c > --- b/src/bin/scripts/clusterdb.c > *************** main(int argc, char *argv[]) > *** 71,83 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 71,83 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 92,110 **** > quiet = true; > break; > case 'd': > ! dbname = optarg; > break; > case 'a': > alldb = true; > break; > case 't': > ! table = optarg; > break; > case 'v': > verbose = true; > break; > case 2: > ! maintenance_db = optarg; > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > --- 92,110 ---- > quiet = true; > break; > case 'd': > ! dbname = pg_strdup(optarg); > break; > case 'a': > alldb = true; > break; > case 't': > ! table = pg_strdup(optarg); > break; > case 'v': > verbose = true; > break; > case 2: > ! maintenance_db = pg_strdup(optarg); > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > diff --git a/src/bin/scripts/createdb.c b/src/bin/scripts/createdb.c > new file mode 100644 > index 91b1a24..4df70cb > *** a/src/bin/scripts/createdb.c > --- b/src/bin/scripts/createdb.c > *************** main(int argc, char *argv[]) > *** 74,86 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 74,86 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 92,119 **** > echo = true; > break; > case 'O': > ! owner = optarg; > break; > case 'D': > ! tablespace = optarg; > break; > case 'T': > ! template = optarg; > break; > case 'E': > ! encoding = optarg; > break; > case 1: > ! lc_collate = optarg; > break; > case 2: > ! lc_ctype = optarg; > break; > case 'l': > ! locale = optarg; > break; > case 3: > ! maintenance_db = optarg; > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > --- 92,119 ---- > echo = true; > break; > case 'O': > ! owner = pg_strdup(optarg); > break; > case 'D': > ! tablespace = pg_strdup(optarg); > break; > case 'T': > ! template = pg_strdup(optarg); > break; > case 'E': > ! encoding = pg_strdup(optarg); > break; > case 1: > ! lc_collate = pg_strdup(optarg); > break; > case 2: > ! lc_ctype = pg_strdup(optarg); > break; > case 'l': > ! locale = pg_strdup(optarg); > break; > case 3: > ! maintenance_db = pg_strdup(optarg); > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > diff --git a/src/bin/scripts/createlang.c b/src/bin/scripts/createlang.c > new file mode 100644 > index 60066af..b85cf04 > *** a/src/bin/scripts/createlang.c > --- b/src/bin/scripts/createlang.c > *************** main(int argc, char *argv[]) > *** 65,77 **** > listlangs = true; > break; > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 65,77 ---- > listlangs = true; > break; > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 80,86 **** > prompt_password = TRI_YES; > break; > case 'd': > ! dbname = optarg; > break; > case 'e': > echo = true; > --- 80,86 ---- > prompt_password = TRI_YES; > break; > case 'd': > ! dbname = pg_strdup(optarg); > break; > case 'e': > echo = true; > diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c > new file mode 100644 > index db3b5d0..d35121b > *** a/src/bin/scripts/createuser.c > --- b/src/bin/scripts/createuser.c > *************** main(int argc, char *argv[]) > *** 89,101 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 89,101 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 139,145 **** > login = TRI_NO; > break; > case 'c': > ! conn_limit = optarg; > break; > case 'P': > pwprompt = true; > --- 139,145 ---- > login = TRI_NO; > break; > case 'c': > ! conn_limit = pg_strdup(optarg); > break; > case 'P': > pwprompt = true; > diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c > new file mode 100644 > index 583655d..5f978cc > *** a/src/bin/scripts/dropdb.c > --- b/src/bin/scripts/dropdb.c > *************** main(int argc, char *argv[]) > *** 64,76 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 64,76 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 88,94 **** > /* this covers the long options */ > break; > case 2: > ! maintenance_db = optarg; > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > --- 88,94 ---- > /* this covers the long options */ > break; > case 2: > ! maintenance_db = pg_strdup(optarg); > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > diff --git a/src/bin/scripts/droplang.c b/src/bin/scripts/droplang.c > new file mode 100644 > index 4772dc5..b9f42bb > *** a/src/bin/scripts/droplang.c > --- b/src/bin/scripts/droplang.c > *************** main(int argc, char *argv[]) > *** 64,76 **** > listlangs = true; > break; > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 64,76 ---- > listlangs = true; > break; > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 79,85 **** > prompt_password = TRI_YES; > break; > case 'd': > ! dbname = optarg; > break; > case 'e': > echo = true; > --- 79,85 ---- > prompt_password = TRI_YES; > break; > case 'd': > ! dbname = pg_strdup(optarg); > break; > case 'e': > echo = true; > diff --git a/src/bin/scripts/dropuser.c b/src/bin/scripts/dropuser.c > new file mode 100644 > index d0bf6ff..7c10101 > *** a/src/bin/scripts/dropuser.c > --- b/src/bin/scripts/dropuser.c > *************** main(int argc, char *argv[]) > *** 62,74 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 62,74 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c > new file mode 100644 > index d1e27bd..f61dada > *** a/src/bin/scripts/reindexdb.c > --- b/src/bin/scripts/reindexdb.c > *************** main(int argc, char *argv[]) > *** 78,90 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 78,90 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 99,105 **** > quiet = true; > break; > case 'd': > ! dbname = optarg; > break; > case 'a': > alldb = true; > --- 99,105 ---- > quiet = true; > break; > case 'd': > ! dbname = pg_strdup(optarg); > break; > case 'a': > alldb = true; > *************** main(int argc, char *argv[]) > *** 108,120 **** > syscatalog = true; > break; > case 't': > ! table = optarg; > break; > case 'i': > ! index = optarg; > break; > case 2: > ! maintenance_db = optarg; > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > --- 108,120 ---- > syscatalog = true; > break; > case 't': > ! table = pg_strdup(optarg); > break; > case 'i': > ! index = pg_strdup(optarg); > break; > case 2: > ! maintenance_db = pg_strdup(optarg); > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c > new file mode 100644 > index 0ac6ab4..eb28ad4 > *** a/src/bin/scripts/vacuumdb.c > --- b/src/bin/scripts/vacuumdb.c > *************** main(int argc, char *argv[]) > *** 82,94 **** > switch (c) > { > case 'h': > ! host = optarg; > break; > case 'p': > ! port = optarg; > break; > case 'U': > ! username = optarg; > break; > case 'w': > prompt_password = TRI_NO; > --- 82,94 ---- > switch (c) > { > case 'h': > ! host = pg_strdup(optarg); > break; > case 'p': > ! port = pg_strdup(optarg); > break; > case 'U': > ! username = pg_strdup(optarg); > break; > case 'w': > prompt_password = TRI_NO; > *************** main(int argc, char *argv[]) > *** 103,109 **** > quiet = true; > break; > case 'd': > ! dbname = optarg; > break; > case 'z': > and_analyze = true; > --- 103,109 ---- > quiet = true; > break; > case 'd': > ! dbname = pg_strdup(optarg); > break; > case 'z': > and_analyze = true; > *************** main(int argc, char *argv[]) > *** 118,124 **** > alldb = true; > break; > case 't': > ! table = optarg; > break; > case 'f': > full = true; > --- 118,124 ---- > alldb = true; > break; > case 't': > ! table = pg_strdup(optarg); > break; > case 'f': > full = true; > *************** main(int argc, char *argv[]) > *** 127,133 **** > verbose = true; > break; > case 2: > ! maintenance_db = optarg; > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > --- 127,133 ---- > verbose = true; > break; > case 2: > ! maintenance_db = pg_strdup(optarg); > break; > default: > fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); > diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c > new file mode 100644 > index 7e7bae3..1b775a1 > *** a/src/interfaces/ecpg/preproc/ecpg.c > --- b/src/interfaces/ecpg/preproc/ecpg.c > *************** main(int argc, char *const argv[]) > *** 171,177 **** > regression_mode = true; > break; > case 'o': > ! output_filename = optarg; > if (strcmp(output_filename, "-") == 0) > yyout = stdout; > else > --- 171,177 ---- > regression_mode = true; > break; > case 'o': > ! output_filename = strdup(optarg); > if (strcmp(output_filename, "-") == 0) > yyout = stdout; > else > diff --git a/src/timezone/zic.c b/src/timezone/zic.c > new file mode 100644 > index 8a95d6a..0aa90eb > *** a/src/timezone/zic.c > --- b/src/timezone/zic.c > *************** main(int argc, char *argv[]) > *** 505,511 **** > usage(stderr, EXIT_FAILURE); > case 'd': > if (directory == NULL) > ! directory = optarg; > else > { > (void) fprintf(stderr, > --- 505,511 ---- > usage(stderr, EXIT_FAILURE); > case 'd': > if (directory == NULL) > ! directory = strdup(optarg); > else > { > (void) fprintf(stderr, > *************** main(int argc, char *argv[]) > *** 516,522 **** > break; > case 'l': > if (lcltime == NULL) > ! lcltime = optarg; > else > { > (void) fprintf(stderr, > --- 516,522 ---- > break; > case 'l': > if (lcltime == NULL) > ! lcltime = strdup(optarg); > else > { > (void) fprintf(stderr, > *************** main(int argc, char *argv[]) > *** 527,533 **** > break; > case 'p': > if (psxrules == NULL) > ! psxrules = optarg; > else > { > (void) fprintf(stderr, > --- 527,533 ---- > break; > case 'p': > if (psxrules == NULL) > ! psxrules = strdup(optarg); > else > { > (void) fprintf(stderr, > *************** main(int argc, char *argv[]) > *** 538,544 **** > break; > case 'y': > if (yitcommand == NULL) > ! yitcommand = optarg; > else > { > (void) fprintf(stderr, > --- 538,544 ---- > break; > case 'y': > if (yitcommand == NULL) > ! yitcommand = strdup(optarg); > else > { > (void) fprintf(stderr, > *************** main(int argc, char *argv[]) > *** 549,555 **** > break; > case 'L': > if (leapsec == NULL) > ! leapsec = optarg; > else > { > (void) fprintf(stderr, > --- 549,555 ---- > break; > case 'L': > if (leapsec == NULL) > ! leapsec = strdup(optarg); > else > { > (void) fprintf(stderr, > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -
Re: getopt() and strdup()
Phil Sorber <phil@omniti.com> — 2012-10-13T19:07:12Z
On Wed, Oct 10, 2012 at 7:54 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Mon, Oct 8, 2012 at 09:03:37PM -0400, Bruce Momjian wrote: >> On Mon, Oct 8, 2012 at 04:33:29PM -0400, Tom Lane wrote: >> > Bruce Momjian <bruce@momjian.us> writes: >> > > A while ago I noticed that in some places we strdup/pg_strdup() optarg >> > > strings from getopt(), and in some places we don't. >> > >> > > If we needed the strdup(), the missing cases should generate errors. If >> > > we don't need them, the strdup() is unnecessary, and research confirms >> > > they are unnecessary. Should we remove the extra strdup/pg_strdup() >> > > calls, for consistency. >> > >> > What research? Given the number of different ways argv[] is handled >> > on different platforms (cf ps_status.c), I am very unwilling to trust >> > that it's safe to hang onto an argv string for long without strdup'ing >> > it. >> > >> > > I think we might have had old platforms that required it, but none are >> > > still supported today. >> > >> > And what's your grounds for stating that? All the alternatives in >> > ps_status.c are still live code AFAICS. >> > >> > My feeling is it's more likely to be a good idea to be adding strdup's >> > than removing them. >> >> Well, what we have now is either wrong or over-kill --- I don't know for >> sure which. > > OK, I have developed the attached patch to add strdup/pg_strdup() calls > to all saving of getopt optarg arguments. > > Also, do we want to centralize the definition of pg_strdup() in /port, > or leave each module to define it on its own? I see pg_strdup() defined > in these modules: > > /pgtop/contrib/oid2name > /pgtop/contrib/pgbench > /pgtop/contrib/pg_upgrade > /pgtop/src/bin/initdb > /pgtop/src/bin/pg_basebackup > /pgtop/src/bin/pg_ctl > /pgtop/src/bin/pg_dump > /pgtop/src/bin/psql > /pgtop/src/bin/scripts > +1 for a centralized definition. > -- > Bruce Momjian <bruce@momjian.us> http://momjian.us > EnterpriseDB http://enterprisedb.com > > + It's impossible for everything to be true. + > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
-
Re: getopt() and strdup()
Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-13T19:14:09Z
Phil Sorber <phil@omniti.com> writes: > On Wed, Oct 10, 2012 at 7:54 PM, Bruce Momjian <bruce@momjian.us> wrote: >> Also, do we want to centralize the definition of pg_strdup() in /port, >> or leave each module to define it on its own? > +1 for a centralized definition. The difficulty with a centralized definition is that it's not clear that the error path is or should be *exactly* the same for all these usages. I see at least six variants right now. While some are gratuitous, some of them are tied into local conventions of each program. regards, tom lane
-
Re: getopt() and strdup()
Phil Sorber <phil@omniti.com> — 2012-10-13T19:28:01Z
On Sat, Oct 13, 2012 at 3:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Phil Sorber <phil@omniti.com> writes: >> On Wed, Oct 10, 2012 at 7:54 PM, Bruce Momjian <bruce@momjian.us> wrote: >>> Also, do we want to centralize the definition of pg_strdup() in /port, >>> or leave each module to define it on its own? > >> +1 for a centralized definition. > > The difficulty with a centralized definition is that it's not clear that > the error path is or should be *exactly* the same for all these usages. > I see at least six variants right now. While some are gratuitous, > some of them are tied into local conventions of each program. > > regards, tom lane Is it possible to at least standardize on one for the front-end and one for the back-end? Then we can use those two going forward and sort out all these other usages and get them to conform to one of the aforementioned definitions over time.