Re: pg_ctl.c

Gaetano Mendola wrote:

> 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.)

It does not appear that you have followed Bruce's instructions above for
testing this. It works just fine for me:

[andrew(at)marmaduke pg_ctl_x]$ make
make -C ../../../src/interfaces/libpq all
make[1]: Entering directory `/home/andrew/pgwnew/pgsql/src/interfaces/libpq'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/andrew/pgwnew/pgsql/src/interfaces/libpq'
make -C ../../../src/port all
make[1]: Entering directory `/home/andrew/pgwnew/pgsql/src/port'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/andrew/pgwnew/pgsql/src/port'
gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes
-Wmissing-declarations -DFRONTEND -DDEF_PGPORT=5432
-I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE
-c -o pg_ctl.o pg_ctl.c
rm -f exec.c && ln -s ../../../src/port/exec.c .
gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes
-Wmissing-declarations -DFRONTEND -DDEF_PGPORT=5432
-I../../../src/interfaces/libpq -I../../../src/include -D_GNU_SOURCE
-c -o exec.o exec.c
gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes
-Wmissing-declarations pg_ctl.o exec.o -L../../../src/interfaces/libpq
-lpq -L../../../src/port -Wl,-rpath,/usr/local/pgsql/lib -lz -lreadline
-ltermcap -lcrypt -lresolv -lnsl -ldl -lm -lbsd -lpgport -o pg_ctl
[andrew(at)marmaduke pg_ctl_x]$

What version of the pg include files is in /usr/include/pgsql/server ?
If <= 7.4 then of course DEVNULL will not be defined.

>
>
> however below the result of my quich review:
>
> 1) exit(1) => exit(EXIT_FAILURE)

If we used a number of different error codes I might agree. But it seems
pointless here, and the style is widely used in our code base (I just
counted 201 other occurrrences, not including cases of exit(0) ).

> 2) xstrdup protected by duplicate NULL string

I don't object, but it is redundant - in every case where it is called
the argument is demonstrably not NULL.

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

True - that's part of the polish needed.

BTW, please don't send reverse diffs, they are a pain to read, IMNSHO
(i.e. you should do diff -c file.c.orig file.c instead of having the
files the other way around).

There is one small thing that is wrong with it - an incorrect format
argument. see patch below.

cheers

andrew

*** pg_ctl.c.orig 2004-05-26 10:27:20.000000000 -0400
--- pg_ctl.c 2004-05-26 10:28:34.000000000 -0400
***************
*** 237,243 ****
*portstr = '\0';

if (getenv("PGPORT") != NULL) /* environment */
! snprintf(portstr, sizeof(portstr), "%d", getenv("PGPORT"));
else /* post_opts */
{
char *p;
--- 237,243 ----
*portstr = '\0';

if (getenv("PGPORT") != NULL) /* environment */
! snprintf(portstr, sizeof(portstr), "%s", getenv("PGPORT"));
else /* post_opts */
{
char *p;