Re: [PATCHES] pg_ctl.c
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Bruce Momjian wrote: | 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.) | | | DEVNULL is in CVS port.h. Are you running against CVS? No. Against 7.4.1 |2) xstrdup protected by duplicate NULL string | | | You mean that you can't pass NULL to xstrdup? Yea, but if we do, it | will crash and we will hear about it right away. Yea u'r right, it will crash without know why, for the same reason you can avoid to use the xstrdup then. Regards Gaetano Mendola -BEGIN PGP SIGNATURE- Version: GnuPG v1.2.4 (MingW32) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFAtE3h7UpzwH2SGd4RAjDhAKDV8lRn7XEAGqHLBJzQTvrBlyJsXgCgrw5c CljcRIysGU3+IGoVbHU9vYg= =9jrs -END PGP SIGNATURE- ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] 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: [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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.0 -0400 --- pg_ctl.c2004-05-26 10:28:34.0 -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; ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] pg_ctl.c
OK, here is a new version with lots of fixes. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 /*- * * pg_ctl --- start/stops/restarts the PostgreSQL server * * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * * $PostgreSQL: pgsql-server/src/bin/initdb/initdb.c,v 1.32 2004/05/18 03:36:36 momjian Exp $ * *- */ #include postgres_fe.h #include libpq-fe.h #include signal.h #include errno.h #include sys/types.h #include sys/stat.h #include libpq/pqsignal.h #include getopt_long.h #ifndef HAVE_OPTRESET int optreset; #endif #define _(x) gettext((x)) #define WHITESPACE \f\n\r\t\v /* as defined by isspace() */ /* postmaster version ident string */ #define PM_VERSIONSTR postmaster (PostgreSQL) PG_VERSION \n typedef enum { SMART_MODE, FAST_MODE, IMMEDIATE_MODE } ShutdownMode; typedef enum { NO_COMMAND = 0, START_COMMAND, STOP_COMMAND, RESTART_COMMAND, RELOAD_COMMAND, STATUS_COMMAND, KILL_COMMAND } CtlCommand; static bool do_wait = false; static bool wait_set = false; static int wait_seconds = 60; static bool silence_echo = false; static ShutdownMode shutdown_mode = SMART_MODE; static int sig = SIGTERM; /* default */ static int killproc; static CtlCommand ctl_command = NO_COMMAND; static char *pg_data_opts = NULL; static char *pg_data = NULL; static char *post_opts = NULL; static const char *progname; static char *log_file = NULL; static char *postgres_path = NULL; static char *argv0 = NULL; static void *xmalloc(size_t size); static char *xstrdup(const char *s); static void do_advice(void); static void do_help(void); static void set_mode(char *modeopt); static void set_sig(char *signame); static void do_start(); static void do_stop(void); static void do_restart(void); static void do_reload(void); static void do_status(void); static void do_kill(void); static long get_pgpid(void); static char **readfile(char *path); static int start_postmaster(void); static bool test_postmaster_connection(void); static char def_postopts_file[MAXPGPATH]; static char postopts_file[MAXPGPATH]; static char pid_file[MAXPGPATH]; static char conf_file[MAXPGPATH]; /* * routines to check memory allocations and fail noisily. */ static void * xmalloc(size_t size) { void *result; result = malloc(size); if (!result) { fprintf(stderr, _(%s: out of memory\n), progname); exit(1); } return result; } static char * xstrdup(const char *s) { char *result; result = strdup(s); if (!result) { fprintf(stderr, _(%s: out of memory\n), progname); exit(1); } return result; } static long get_pgpid(void) { FILE *pidf; longpid; pidf = fopen(pid_file, r); if (pidf == NULL) { /* No pid file, not an error on startup */ if (errno == ENOENT) return 0; else { perror(openning pid file); exit(1); } } fscanf(pidf, %ld, pid); fclose(pidf); return pid; } /* * get the lines from a text file - return NULL if file can't be opened */ static char ** readfile(char *path) { FILE *infile; int maxlength = 0, linelen = 0; int nlines = 0; char **result; char *buffer; int c; if ((infile = fopen(path, r)) == NULL) return NULL; /* pass over the file twice - the first time to size the result */ while ((c = fgetc(infile)) != EOF) { linelen++; if (c == '\n') { nlines++; if (linelen maxlength) maxlength = linelen; linelen = 0; } } /* handle last line without a terminating newline (yuck) */ if (linelen) nlines++; if (linelen maxlength) maxlength = linelen; /* set up the result and the line buffer */ result = (char **) xmalloc((nlines + 1) * sizeof(char *)); buffer = (char *) xmalloc(maxlength + 1); /* now reprocess the file and store the lines */ rewind(infile);
Re: [PATCHES] pg_ctl.c
Andrew Dunstan wrote: Gaetano Mendola wrote: Bruce Momjian wrote: 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) ). This doesn't mean that we don't have to. 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. Now it's true, and in the future ? Bruce was arguing about that check that if the string is null the program simply will exit crashing! I really appreciate the quality software of Postgres but some time I don't understand why test NULL pointer is an overkill for you. I mean xstrdup is supposed to be the strdup safe version, and without that control is not safe, why don't use directly the strdup then ? If there is no memory available before postgresql start go figure after! Regards Gaetano Mendola ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] pg_ctl.c
Gaetano Mendola wrote: Andrew Dunstan wrote: Gaetano Mendola wrote: Bruce Momjian wrote: 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) ). This doesn't mean that we don't have to. We should be consistent. If you want to prepare a global patch that replaces every instance of exit(n) with exit(SOME_CONSTANT) then be my guest. 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. Now it's true, and in the future ? Bruce was arguing about that check that if the string is null the program simply will exit crashing! I really appreciate the quality software of Postgres but some time I don't understand why test NULL pointer is an overkill for you. I mean xstrdup is supposed to be the strdup safe version, and without that control is not safe, why don't use directly the strdup then ? If there is no memory available before postgresql start go figure after! I am not arguing that we should crash. I am arguing that we will not crash in this case, and the test is therefore redundant. I already said I don't object to the change, if that's the consensus, just that it is unnecessary. BTW, this code was lifted directly from initdb.c. I'd far rather you found real bugs than the ghosts of imaginary bugs, though. cheers andrew ---(end of broadcast)--- TIP 8: explain analyze is your friend
[PATCHES] pg_ctl.c
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! -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 /*- * * pg_ctl --- start/stops/restarts the PostgreSQL server * * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * * $PostgreSQL: pgsql-server/src/bin/initdb/initdb.c,v 1.32 2004/05/18 03:36:36 momjian Exp $ * *- */ #include libpq-fe.h #include postgres_fe.h #include signal.h #include errno.h #include libpq/pqsignal.h #include string.h #include sys/types.h #include sys/stat.h #define _(x) gettext((x)) #define WHITESPACE \f\n\r\t\v /* as defined by isspace() */ /* postmaster version ident string */ #define PM_VERSIONSTR postmaster (PostgreSQL) PG_VERSION \n typedef enum { SMART_MODE, FAST_MODE, IMMEDIATE_MODE } ShutdownMode; typedef enum { NO_COMMAND = 0, START_COMMAND, STOP_COMMAND, RESTART_COMMAND, RELOAD_COMMAND, STATUS_COMMAND, KILL_COMMAND } CtlCommand; static bool do_wait = false; static bool wait_set = false; static int wait_seconds = 60; static bool silence_echo = false; static ShutdownMode shutdown_mode = SMART_MODE; static int sig = SIGTERM; /* default */ static int killproc; static CtlCommand ctl_command = NO_COMMAND; static char *pg_data_opts = NULL; static char *pg_data = NULL; static char *post_opts = NULL; static const char *progname; static char *log_file = NULL; static char *postgres_path = NULL; static char *argv0 = NULL; static void *xmalloc(size_t size); static char *xstrdup(const char *s); static void do_advice(void); static void do_help(void); static void set_mode(char *modeopt); static void set_sig(char *signame); static void do_start(); static void do_stop(void); static void do_restart(void); static void do_reload(void); static void do_status(void); static void do_kill(void); static long get_pgpid(void); static char **readfile(char *path); static void start_postmaster(void); static bool test_postmaster_connection(void); static char def_postopts_file[MAXPGPATH]; static char postopts_file[MAXPGPATH]; static char pid_file[MAXPGPATH]; static char conf_file[MAXPGPATH]; /* * routines to check memory allocations and fail noisily. */ static void * xmalloc(size_t size) { void *result; result = malloc(size); if (!result) { fprintf(stderr, _(%s: out of memory\n), progname); exit(1); } return result; } static char * xstrdup(const char *s) { char *result; result = strdup(s); if (!result) { fprintf(stderr, _(%s: out of memory\n), progname); exit(1); } return result; } static long get_pgpid(void) { FILE *pidf; longpid; pidf = fopen(pid_file, r); if (pidf == NULL) { /* No pid file, not an error on startup */ if (errno == ENOENT) return 0; else { perror(openning pid file); exit(1); } } fscanf(pidf, %ld, pid); fclose(pidf); return pid; } /* * get the lines from a text file - return NULL if file can't be opened */ static char ** readfile(char *path) { FILE *infile; int maxlength = 0, linelen = 0; int nlines = 0; char **result; char *buffer; int c; if ((infile = fopen(path, r)) == NULL) return NULL; /* pass over the file twice - the first time to size the result */ while ((c = fgetc(infile)) != EOF) { linelen++; if (c == '\n') { nlines++; if (linelen maxlength) maxlength = linelen; linelen = 0; } } /* handle last line without a terminating newline (yuck) */ if (linelen) nlines++; if (linelen maxlength) maxlength = linelen; /* set up the
Re: [PATCHES] 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.c2004-05-26 02:48:38.0 +0200 --- pg_ctl.c.orig 2004-05-26 02:43:32.0 +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