Bug #705
closedVKERNEL Pidfile patch
0%
Description
Okay.. So heres a quick patch to have the vkernel write out a pidfile.
(WFM-TM)
3 possible 'messies':
- doing this 'inline' within the getopt processing. Felt this
was simpleenough a task that that wasn't a big deal, but I
can see how some might not like it that way.
- If the pidfile isn't writable, it's considered a warning and not
an error. Was using inetd as my inspiration, and kept
things consistent - but perhaps this shouldn't be the case?
- the vkernel.7 patch was manually edited to remove the signal edits
from earlier.. hopefully it still applies cleanly to HEAD. Only a
sentance or two there probably not too much to fix if so.
Comments / Concerns / etc. welcome - didn't edit Nuno's rc script as he
said he'd take care of that and I'm using another mechanism to handle
VKERNEL systems at the moment.
Thanks,
- Chris
Files
Updated by joerg almost 17 years ago
How does this work if -p is specified more than once?
You don't have to strdup optarg if you don't modify argv.
Joerg
Updated by c.turner almost 17 years ago
good call on -pp .. will fix up.
on the strdup -
was matching up the other getopt parse styles..
some of those don't appear to modify argv either..
Updated by c.turner almost 17 years ago
Updated version:
- only handles 1 pidfile, which is the last -p argument specified.
- pidfile processing moved to function for clarity
- non-strduping of optarg
Thanks,
- Chris
Updated by dillon almost 17 years ago
This is the ~/.cvsrc file we recommend:
cvs -q
diff -u
update -Pd
checkout -P
rtag -a
-Matt
Updated by c.turner almost 17 years ago
oops.. hadn't had my coffee yet :) See attached.
as mentioned in the first post, this is a manually edited patch as
I've got another one outstanding on the vkernel.7 file for the
vk-shutdown..
(which I'll check for same diff -u issue)
Perhaps I should submit them as one - but trying to keep the features
separate..
Thanks,
- Chris
Updated by joerg almost 17 years ago
if (pidfile != NULL)
+ writepid(pidfile);
cpu_disable_intr();
init_sys_memory(memImageFile);
init_kern_memory();
... otherwise you get a segmenation fault.
Joerg
Updated by c.turner almost 17 years ago
Joerg Sonnenberger wrote:
doh!
I must be in m-x moron-mode today..
will wait some time to see if there's any more shakeout / suggestions
before resubmitting.
thanks.
- Chris
Updated by nant almost 17 years ago
Wiki updated to reflect this:
http://wiki.dragonflybsd.org/index.cgi/HandyHints#head-4d3ffe00189ec577f0ea51caa853e61bcb2962a7
Thanks,
Nuno
Updated by corecode almost 17 years ago
style(9) says:
Test pointers against NULL, e.g., use:
(p = f()) == NULL
not:
!(p = f())
so, make this
if (pidfile != NULL)
plus, do you think the vkernel should clean up the pidfile when shutting down?
cheers
simon
Updated by dillon almost 17 years ago
:so, make this
:if (pidfile !=3D NULL)
:
:plus, do you think the vkernel should clean up the pidfile when shutting =
:down?
:
:cheers
: simon
It's a good idea. And easy for it to do.
-Matt
Updated by hasso almost 17 years ago
I'd add -p to diff. It sometimes helps a lot.
Updated by c.turner almost 17 years ago
Okay.. as suggested.
Updated version:
- 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.
vkernel.7 diff is still against previous version, but as-cleaned,
should apply OK.
Thanks,
- Chris
Updated by dillon almost 17 years ago
: - 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>
Updated by c.turner almost 17 years ago
Nearly did this on the first round, but thought it wasn't as legible,
and wasn't sure what the consensus here was..
As on occasional perl programmer, I like my statements short and cryptic
:)
Okay.. whew. fine-tooth-comb here..
but it does look much cleaner, I will admit.
Thanks for the reviews, esp for such a simple patch.
- Chris
Updated by dillon almost 17 years ago
:Okay.. whew. fine-tooth-comb here..
:
:but it does look much cleaner, I will admit.
:
:Thanks for the reviews, esp for such a simple patch.
:
:- Chris
That looks good. I'm gonna commit it.
-Matt
Updated by c.turner almost 17 years ago
woohoo! just saw the commits. thanks!
Now for that vktty thing.. (eventually)
- Chris