Thread: Re: [pgsql-hackers-win32] pg_ctl
[ Adding patches list.] Well, I would change the // and use /* */, and remove the C++ code so it is straight C. Also, it should look more like our code, like initdb.c does. --------------------------------------------------------------------------- Joshua D. Drake wrote: > Hello, > > Here is alpha2 of pg_ctl. It is written to work on Linux(unix) and > Win32. Please let me know > if we missed anything. > > http://www.commandprompt.com/files/pg_ctl.tar.gz > > Sincerely, > > Joshua D. Drake > > -- > Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC > Postgresql support, programming shared hosting and dedicated hosting. > +1-503-667-4564 - jd@commandprompt.com - http://www.commandprompt.com > Mammoth PostgreSQL Replicator. Integrated Replication for PostgreSQL > > > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faqs/FAQ.html > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
This code is pretty awful, IMHO.
If you're going to copy code from a 3rd party (in this case, MSDN), it
is standard practise to include an attribution. Also, what
redistribution terms apply to MSDN sample code?
Assuming we don't remove or rewrite the MSDN sample code, it is
usually considered good practise to put imported code into a separate
file.
You can remove the kludge for 16-bit processes, as PostgreSQL won't
ever be one. Dunno if it's worth modifying the sample code for this.
It is generally considered good practise to divide distinct
functionality into separate functions, rather than use a ~450 line
main() function.
I'd like to see fewer WIN32 #ifdefs in the main code path; please also
endeavour to make them as small as possible.
Copying and pasting code from one branch of an #ifdef or if into the
other should also be avoided where possible: this is done in a couple
places.
Other random badness I noticed while browsing through the code:
#ifdef WIN32
char *bindir="c:\\pgsql\\bin";
char *VERSION="7.3.5";
#else
char *bindir="@bindir@";
char *VERSION="@VERSION@";
#endif
This is plainly wrong.
char* DEFPOSTOPTS= NULL;
char* POSTOPTSFILE= NULL;
char* PIDFILE= NULL;
int n_sig;
char *logfile=NULL;
char *silence_echo=NULL;
char *shutdown_mode="smart";
char *op=NULL;
int PID;
Can we please try to follow *some* kind of coherent convention for
naming and indentation?
char* buffer;
char* buffer2;
Please put some thought into your variable names.
po_path=(char*)malloc(sizeof(char)* (strlen(PGPATH)+strlen(pgsqlexe)+2));
sprintf(po_path, "%s%s%s",PGPATH, path_delim, pgsqlexe);
This code needlessly assumes strlen(path_delim) == 1. On the other
hand, it is reasonable to assume sizeof(char) == 1 (this is guaranteed
by ANSI C).
for ( ;cont; token = (char*)strtok(NULL, env_delim)){
if (token==NULL) cont=0; else {
buffer=(char*)malloc(sizeof(char)* (strlen(token)+strlen(CMDNAME)+1));
sprintf(buffer, token);
strcat (buffer, path_delim);
strcat (buffer, CMDNAME);
if (post_file=fopen(buffer, "r")){
self_path=token;
cont=0;
free(buffer);
} else free(buffer);
}
if (token==NULL) cont=0;
}
Good lord.
-Neil
Neil Conway <neilc@samurai.com> writes: > This code is pretty awful, IMHO. Also, why is this code still invoking psql manually? ISTM that linking against libpq directly would be much better. -Neil
Yea, I have to agree. It looks like it was written by an MS C
programmer, not by a PostgreSQL programmer in terms of style and
structure.
Sorry for the bad news.
---------------------------------------------------------------------------
Neil Conway wrote:
> This code is pretty awful, IMHO.
>
> If you're going to copy code from a 3rd party (in this case, MSDN), it
> is standard practise to include an attribution. Also, what
> redistribution terms apply to MSDN sample code?
>
> Assuming we don't remove or rewrite the MSDN sample code, it is
> usually considered good practise to put imported code into a separate
> file.
>
> You can remove the kludge for 16-bit processes, as PostgreSQL won't
> ever be one. Dunno if it's worth modifying the sample code for this.
>
> It is generally considered good practise to divide distinct
> functionality into separate functions, rather than use a ~450 line
> main() function.
>
> I'd like to see fewer WIN32 #ifdefs in the main code path; please also
> endeavour to make them as small as possible.
>
> Copying and pasting code from one branch of an #ifdef or if into the
> other should also be avoided where possible: this is done in a couple
> places.
>
> Other random badness I noticed while browsing through the code:
>
> #ifdef WIN32
> char *bindir="c:\\pgsql\\bin";
> char *VERSION="7.3.5";
> #else
> char *bindir="@bindir@";
> char *VERSION="@VERSION@";
> #endif
>
> This is plainly wrong.
>
> char* DEFPOSTOPTS= NULL;
> char* POSTOPTSFILE= NULL;
> char* PIDFILE= NULL;
>
> int n_sig;
>
> char *logfile=NULL;
> char *silence_echo=NULL;
> char *shutdown_mode="smart";
> char *op=NULL;
> int PID;
>
> Can we please try to follow *some* kind of coherent convention for
> naming and indentation?
>
> char* buffer;
> char* buffer2;
>
> Please put some thought into your variable names.
>
> po_path=(char*)malloc(sizeof(char)* (strlen(PGPATH)+strlen(pgsqlexe)+2));
> sprintf(po_path, "%s%s%s",PGPATH, path_delim, pgsqlexe);
>
> This code needlessly assumes strlen(path_delim) == 1. On the other
> hand, it is reasonable to assume sizeof(char) == 1 (this is guaranteed
> by ANSI C).
>
> for ( ;cont; token = (char*)strtok(NULL, env_delim)){
> if (token==NULL) cont=0; else {
> buffer=(char*)malloc(sizeof(char)* (strlen(token)+strlen(CMDNAME)+1));
> sprintf(buffer, token);
> strcat (buffer, path_delim);
> strcat (buffer, CMDNAME);
> if (post_file=fopen(buffer, "r")){
> self_path=token;
> cont=0;
> free(buffer);
> } else free(buffer);
> }
> if (token==NULL) cont=0;
> }
>
> Good lord.
>
> -Neil
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Hello, New version will be resolved next week. Sincerely, Joshua D. Drake Bruce Momjian wrote: >[ Adding patches list.] > >Well, I would change the // and use /* */, and remove the C++ code so it >is straight C. Also, it should look more like our code, like initdb.c >does. > >--------------------------------------------------------------------------- > >Joshua D. Drake wrote: > > >>Hello, >> >> Here is alpha2 of pg_ctl. It is written to work on Linux(unix) and >>Win32. Please let me know >>if we missed anything. >> >>http://www.commandprompt.com/files/pg_ctl.tar.gz >> >>Sincerely, >> >>Joshua D. Drake >> >>-- >>Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC >>Postgresql support, programming shared hosting and dedicated hosting. >>+1-503-667-4564 - jd@commandprompt.com - http://www.commandprompt.com >>Mammoth PostgreSQL Replicator. Integrated Replication for PostgreSQL >> >> >> >>---------------------------(end of broadcast)--------------------------- >>TIP 5: Have you checked our extensive FAQ? >> >> http://www.postgresql.org/docs/faqs/FAQ.html >> >> >> > > > -- Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC Postgresql support, programming shared hosting and dedicated hosting. +1-503-222-2783 - jd@commandprompt.com - http://www.commandprompt.com Editor-N-Chief - PostgreSQl.Org - http://www.postgresql.org
Hello,
I am addressing these issues with my programmers now. Should see
better results next week.
Sincerely,
Joshua D. Drake
Neil Conway wrote:
>This code is pretty awful, IMHO.
>
>If you're going to copy code from a 3rd party (in this case, MSDN), it
>is standard practise to include an attribution. Also, what
>redistribution terms apply to MSDN sample code?
>
>Assuming we don't remove or rewrite the MSDN sample code, it is
>usually considered good practise to put imported code into a separate
>file.
>
>You can remove the kludge for 16-bit processes, as PostgreSQL won't
>ever be one. Dunno if it's worth modifying the sample code for this.
>
>It is generally considered good practise to divide distinct
>functionality into separate functions, rather than use a ~450 line
>main() function.
>
>I'd like to see fewer WIN32 #ifdefs in the main code path; please also
>endeavour to make them as small as possible.
>
>Copying and pasting code from one branch of an #ifdef or if into the
>other should also be avoided where possible: this is done in a couple
>places.
>
>Other random badness I noticed while browsing through the code:
>
> #ifdef WIN32
> char *bindir="c:\\pgsql\\bin";
> char *VERSION="7.3.5";
> #else
> char *bindir="@bindir@";
> char *VERSION="@VERSION@";
> #endif
>
>This is plainly wrong.
>
> char* DEFPOSTOPTS= NULL;
> char* POSTOPTSFILE= NULL;
> char* PIDFILE= NULL;
>
> int n_sig;
>
> char *logfile=NULL;
> char *silence_echo=NULL;
> char *shutdown_mode="smart";
> char *op=NULL;
> int PID;
>
>Can we please try to follow *some* kind of coherent convention for
>naming and indentation?
>
> char* buffer;
> char* buffer2;
>
>Please put some thought into your variable names.
>
> po_path=(char*)malloc(sizeof(char)* (strlen(PGPATH)+strlen(pgsqlexe)+2));
> sprintf(po_path, "%s%s%s",PGPATH, path_delim, pgsqlexe);
>
>This code needlessly assumes strlen(path_delim) == 1. On the other
>hand, it is reasonable to assume sizeof(char) == 1 (this is guaranteed
>by ANSI C).
>
> for ( ;cont; token = (char*)strtok(NULL, env_delim)){
> if (token==NULL) cont=0; else {
> buffer=(char*)malloc(sizeof(char)* (strlen(token)+strlen(CMDNAME)+1));
> sprintf(buffer, token);
> strcat (buffer, path_delim);
> strcat (buffer, CMDNAME);
> if (post_file=fopen(buffer, "r")){
> self_path=token;
> cont=0;
> free(buffer);
> } else free(buffer);
> }
> if (token==NULL) cont=0;
> }
>
>Good lord.
>
>-Neil
>
>
--
Command Prompt, Inc., home of Mammoth PostgreSQL - S/ODBC and S/JDBC
Postgresql support, programming shared hosting and dedicated hosting.
+1-503-222-2783 - jd@commandprompt.com - http://www.commandprompt.com
Editor-N-Chief - PostgreSQl.Org - http://www.postgresql.org