: - 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
<[EMAIL PROTECTED]>
