Re: pg_ctl.c - Mailing list pgsql-patches

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@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@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;