Re: pg_ctl.c - Mailing list pgsql-patches

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)