Re: pg_ctl.c

Bruce Momjian wrote:

> Here is the C version of pg_ctl.c written by Andrew Dunstan and updated
> by me.
>
> You can use it by creating a src/bin/pg_ctl_test directory and putting
> the C and Makefile into that directory. You can then do a make install
> and use it for testing.
>
> Unless someone finds a problem, I will apply the change soon. This
> removes our last shell script!

It desn't compile on my platform:

$ gcc -I /usr/include/pgsql/server pg_ctl.c
pg_ctl.c: In function `start_postmaster':
pg_ctl.c:219: error: `DEVNULL' undeclared (first use in this function)
pg_ctl.c:219: error: (Each undeclared identifier is reported only once
pg_ctl.c:219: error: for each function it appears in.)

however below the result of my quich review:

1) exit(1) => exit(EXIT_FAILURE)
2) xstrdup protected by duplicate NULL string

I seen also that you don't use always the _ macro for error display.

Regards
Gaetano Mendola

*** pg_ctl.c 2004-05-26 02:48:38.000000000 +0200
--- pg_ctl.c.orig 2004-05-26 02:43:32.000000000 +0200
***************
*** 26,33 ****
/* postmaster version ident string */
#define PM_VERSIONSTR "postmaster (PostgreSQL) " PG_VERSION "\n"

- #define EXIT_FAILURE 1
-

typedef enum
{
--- 26,31 ----
***************
*** 100,106 ****
if (!result)
{
fprintf(stderr, _("%s: out of memory\n"), progname);
! exit(EXIT_FAILURE);
}
return result;
}
--- 98,104 ----
if (!result)
{
fprintf(stderr, _("%s: out of memory\n"), progname);
! exit(1);
}
return result;
}
***************
*** 112,130 ****
{
char *result;

-
- if (!s)
- {
- fprintf(stderr, "%s: can not duplicate null pointer", progname);
- exit(EXIT_FAILURE);
- }
-
-
result = strdup(s);
if (!result)
{
fprintf(stderr, _("%s: out of memory\n"), progname);
! exit(EXIT_FAILURE);
}
return result;
}
--- 110,120 ----
{
char *result;

result = strdup(s);
if (!result)
{
fprintf(stderr, _("%s: out of memory\n"), progname);
! exit(1);
}
return result;
}
***************
*** 146,152 ****
else
{
perror("openning pid file");
! exit(EXIT_FAILURE);
}
}
fscanf(pidf, "%ld", &pid);
--- 136,142 ----
else
{
perror("openning pid file");
! exit(1);
}
}
fscanf(pidf, "%ld", &pid);
***************
*** 353,359 ****
else
{
fprintf(stderr, "%s: cannot read %s\n", progname, postopts_file);
! exit(EXIT_FAILURE);
}
}
else if (optlines[0] == NULL || optlines[1] != NULL)
--- 343,349 ----
else
{
fprintf(stderr, "%s: cannot read %s\n", progname, postopts_file);
! exit(1);
}
}
else if (optlines[0] == NULL || optlines[1] != NULL)
***************
*** 361,367 ****
fprintf(stderr, "%s: option file %s must have exactly 1 line\n",
progname, ctl_command == RESTART_COMMAND ?
postopts_file : def_postopts_file);
! exit(EXIT_FAILURE);
}
else
{
--- 351,357 ----
fprintf(stderr, "%s: option file %s must have exactly 1 line\n",
progname, ctl_command == RESTART_COMMAND ?
postopts_file : def_postopts_file);
! exit(1);
}
else
{
***************
*** 411,417 ****
"but was not the same version as \"%s\".\n"
"Check your installation.\n"),
progname, progname);
! exit(EXIT_FAILURE);
}
postgres_path = postmaster_path;
}
--- 401,407 ----
"but was not the same version as \"%s\".\n"
"Check your installation.\n"),
progname, progname);
! exit(1);
}
postgres_path = postmaster_path;
}
***************
*** 428,434 ****
"%s: cannot start postmaster\n"
"Examine the log output\n",
progname);
! exit(EXIT_FAILURE);
}
}

--- 418,424 ----
"%s: cannot start postmaster\n"
"Examine the log output\n",
progname);
! exit(1);
}
}

***************
*** 463,469 ****
{
fprintf(stderr, "%s: could not find %s\n", progname, pid_file);
fprintf(stderr, "Is postmaster running?\n");
! exit(EXIT_FAILURE);
}
else if (pid < 0) /* standalone backend, not postmaster */
{
--- 453,459 ----
{
fprintf(stderr, "%s: could not find %s\n", progname, pid_file);
fprintf(stderr, "Is postmaster running?\n");
! exit(1);
}
else if (pid < 0) /* standalone backend, not postmaster */
{
***************
*** 472,478 ****
"%s: cannot stop postmaster; "
"postgres is running (PID: %ld)\n",
progname, pid);
! exit(EXIT_FAILURE);
}

if (kill((pid_t) pid, sig) != 0)
--- 462,468 ----
"%s: cannot stop postmaster; "
"postgres is running (PID: %ld)\n",
progname, pid);
! exit(1);
}

if (kill((pid_t) pid, sig) != 0)
***************
*** 513,519 ****
printf(" failed\n");

fprintf(stderr, "%s: postmaster does not shut down\n", progname);
! exit(EXIT_FAILURE);
}
}
}
--- 503,509 ----
printf(" failed\n");

fprintf(stderr, "%s: postmaster does not shut down\n", progname);
! exit(1);
}
}
}
***************
*** 546,552 ****
"postgres is running (PID: %ld)\n",
progname, pid);
fprintf(stderr, "Please terminate postgres and try again.\n");
! exit(EXIT_FAILURE);
}

if (kill((pid_t) pid, sig) != 0)
--- 536,542 ----
"postgres is running (PID: %ld)\n",
progname, pid);
fprintf(stderr, "Please terminate postgres and try again.\n");
! exit(1);
}

if (kill((pid_t) pid, sig) != 0)
***************
*** 581,587 ****
printf(" failed\n");

fprintf(stderr, "%s: postmaster does not shut down\n", progname);
! exit(EXIT_FAILURE);
}

do_start();
--- 571,577 ----
printf(" failed\n");

fprintf(stderr, "%s: postmaster does not shut down\n", progname);
! exit(1);
}

do_start();
***************
*** 599,605 ****
{
fprintf(stderr, "%s: could not find %s\n", progname, pid_file);
fprintf(stderr, "Is postmaster running?\n");
! exit(EXIT_FAILURE);
}
else if (pid < 0) /* standalone backend, not postmaster */
{
--- 589,595 ----
{
fprintf(stderr, "%s: could not find %s\n", progname, pid_file);
fprintf(stderr, "Is postmaster running?\n");
! exit(1);
}
else if (pid < 0) /* standalone backend, not postmaster */
{
***************
*** 609,615 ****
"postgres is running (PID: %ld)\n",
progname, pid);
fprintf(stderr, "Please terminate postgres and try again.\n");
! exit(EXIT_FAILURE);
}

if (kill((pid_t) pid, sig) != 0)
--- 599,605 ----
"postgres is running (PID: %ld)\n",
progname, pid);
fprintf(stderr, "Please terminate postgres and try again.\n");
! exit(1);
}

if (kill((pid_t) pid, sig) != 0)
***************
*** 632,638 ****
if (pid == 0) /* no pid file */
{
fprintf(stderr, "%s: postmaster or postgres not running", progname);
! exit(EXIT_FAILURE);
}
else if (pid < 0) /* standalone backend */
{
--- 622,628 ----
if (pid == 0) /* no pid file */
{
fprintf(stderr, "%s: postmaster or postgres not running", progname);
! exit(1);
}
else if (pid < 0) /* standalone backend */
{
***************
*** 745,751 ****
{
fprintf(stderr, "%s: invalid shutdown mode %s\n", progname, modeopt);
do_advice();
! exit(EXIT_FAILURE);
}
}

--- 735,741 ----
{
fprintf(stderr, "%s: invalid shutdown mode %s\n", progname, modeopt);
do_advice();
! exit(1);
}
}

***************
*** 778,784 ****
{
fprintf(stderr, "%s: invalid signal \"%s\"\n", progname, signame);
do_advice();
! exit(EXIT_FAILURE);
}

}
--- 768,774 ----
{
fprintf(stderr, "%s: invalid signal \"%s\"\n", progname, signame);
do_advice();
! exit(1);
}

}
***************
*** 878,884 ****
{
fprintf(stderr, "%s: invalid option %s\n", progname, arg);
do_advice();
! exit(EXIT_FAILURE);
}
else if (strcmp(arg, "start") == 0 && ctl_command == NO_COMMAND)
ctl_command = START_COMMAND;
--- 868,874 ----
{
fprintf(stderr, "%s: invalid option %s\n", progname, arg);
do_advice();
! exit(1);
}
else if (strcmp(arg, "start") == 0 && ctl_command == NO_COMMAND)
ctl_command = START_COMMAND;
***************
*** 902,908 ****
{
fprintf(stderr, "%s: invalid operation mode %s\n", progname, arg);
do_advice();
! exit(EXIT_FAILURE);
}
}

--- 892,898 ----
{
fprintf(stderr, "%s: invalid operation mode %s\n", progname, arg);
do_advice();
! exit(1);
}
}

***************
*** 910,916 ****
{
fprintf(stderr, "%s: no operation specified\n", progname);
do_advice();
! exit(EXIT_FAILURE);
}

pg_data = getenv("PGDATA");
--- 900,906 ----
{
fprintf(stderr, "%s: no operation specified\n", progname);
do_advice();
! exit(1);
}

pg_data = getenv("PGDATA");
***************
*** 923,929 ****
"and environment variable PGDATA unset\n",
progname);
do_advice();
! exit(EXIT_FAILURE);
}

if (!wait_set)
--- 913,919 ----
"and environment variable PGDATA unset\n",
progname);
do_advice();
! exit(1);
}

if (!wait_set)