Skip site navigation (1) Skip section navigation (2)

Re: [PATCHES] pg_ctl

From: Neil Conway <neilc(at)samurai(dot)com>
To: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
Cc: "Joshua D(dot) Drake" <jd(at)commandprompt(dot)com>,PostgreSQL Win32 port list <pgsql-hackers-win32(at)postgresql(dot)org>,PostgreSQL-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: [PATCHES] pg_ctl
Date: 2003-11-29 00:10:47
Message-ID: 87smk8japk.fsf@mailbox.samurai.com (view raw or flat)
Thread:
Lists: pgsql-hackers-win32pgsql-patches
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


In response to

  • Re: pg_ctl at 2003-11-28 22:20:37 from Bruce Momjian

Responses

pgsql-patches by date

Next:From: Neil ConwayDate: 2003-11-29 00:25:58
Subject: Re: [PATCHES] pg_ctl
Previous:From: Bruce MomjianDate: 2003-11-28 22:20:37
Subject: Re: pg_ctl

pgsql-hackers-win32 by date

Next:From: Neil ConwayDate: 2003-11-29 00:25:58
Subject: Re: [PATCHES] pg_ctl
Previous:From: Bruce MomjianDate: 2003-11-28 22:20:37
Subject: Re: pg_ctl

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group