Re: VKERNEL Pidfile patch

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:  - globalized the pidfile char*
:  - reworked writepid() to check for NULL
:    (consistant with other arg-checking within the file)
:  - added cleanpid() and added to halt hooks.

    Looks good. Here's a critique.  No need to initialize a
    global to NULL, it will already be NULL.

:+char *pid_file = NULL;

    No need to initialize self and file to 0/NULL.

:+writepid( void )
:+{
:+	pid_t self = 0;
:+	FILE *file = NULL;

    Unless it creates a serious indenting issue (which sometimes happens,
    but not in this case), don't short-cut the conditional with a return.
    The standard variable name for a stdio file in simple circumstances
    is 'fp' rather then 'file'.  Do this instead:

    pid_t self;
    FILE *fp;

    if (pid_file) {
	    self = getpid();
	    fp = fopen(pid_file, "w");

	    if (fp != NULL) {
		    fprintf(fp, "%ld\n", (long)self);
		    fclose(fp);
	    } else {
		    perror("Warning: couldn't open pidfile");
	    }
    }

:+
:+	if (pid_file == NULL)
:+		return;
:+
:+	self = getpid();
:+	file = fopen(pid_file, "w");
:+
:+	if (file != NULL) {
:+		fprintf(file, "%ld\n", (long)self);
:+		fclose(file);
:+	}
:+	else {
:+		perror("Warning: couldn't open pidfile");
:+	}
:+}

    Same thing below.  Change the coding to:

    if (pid_file != NULL) {
	    /* ignore any error */
	    unlink(pid_file);
	    pid_file = NULL;
    }

:+static
:+void
:+cleanpid( void ) 
:+{
:+	if (pid_file == NULL)
:+		return;
:+	if ( unlink(pid_file) != 0 )
:+		perror("Warning: couldn't remove pidfile");
:+}

    One last note on conditionals like if (pid_file != NULL) { }...
    comparing explicitly against NULL verses just using if (pid_file) { }.

    For simple conditionals such as the above it isn't a big deal.  In more
    complex code it does make the code more readable.  For example, take
    these four ways of doing the same thing:

    if (fp = fopen(pid_file, "w")) {
	    do stuff
	    do stuff
    } else {
	    perror(...)
    }

	This method was commonly used a decade ago, but led to major
	programming mistakes when people would accidently use '=' instead
	of '==' in things like if (a = b) { ... } when they really meant
	if (a == b) { ... }.  Because of that, GCC now reports a warning
	for this case and nobody uses it any more.



    if ((fp = fopen(pid_file, "w")) != NULL) {
	    do stuff
	    do stuff
    } else {
	    perror(...)
    }

	I still do this.  Some people really hate it and for a complex
	operation it probably does end up being less readable.  If it 
	doesn't break the line, though, I consider it ok to still use this
	construct at least for simple code paths.



    fp = fopen(pid_file, "w");
    if (fp) {					<<< TOSS UP, THIS LOOKS BETTER
	    do stuff
	    do stuff
    } else {
	    perror(...)
    }

    fp = fopen(pid_file, "w");
    if (fp != NULL) {				<<< TOSS UP
	    do stuff
	    do stuff
    } else {
	    perror(...)
    }

	For simple code paths it is pretty clear that you do not need 
	the '!= NULL' part.  For more complex code paths, for example where
	the assignment is way far away from the conditional, then the code
	is often more readable if the != NULL is included because it makes
	it obvious that a pointer is being tested.

    if (more->complex->expression) {
	...
    }

    if (more->complex->expression != NULL) {		<<< BETTER
	...
    }

					-Matt
					Matthew Dillon 
					<dillon@backplane.com>