Обсуждение: getopt() and strdup()

Поиск
Список
Период
Сортировка

getopt() and strdup()

От
Bruce Momjian
Дата:
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
Дата:
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
Дата:
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
Дата:
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
Дата:
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
Дата:
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
Дата:
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
Дата:
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
Дата:
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.