Project

General

Profile

Actions

Bug #705

closed

VKERNEL Pidfile patch

Added by c.turner almost 17 years ago. Updated over 16 years ago.

Status:
Closed
Priority:
Low
Assignee:
-
Category:
-
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:

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

vkpid.patch (1.07 KB) vkpid.patch c.turner, 06/17/2007 04:28 AM
vkpid.patch (1.21 KB) vkpid.patch c.turner, 06/17/2007 02:21 PM
vkpid.patch (2.43 KB) vkpid.patch c.turner, 06/17/2007 04:19 PM
vkpid.patch (3 KB) vkpid.patch c.turner, 06/18/2007 12:53 AM
vkpid.patch (2.97 KB) vkpid.patch c.turner, 06/19/2007 12:55 AM
Actions #1

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

Actions #2

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..

Actions #3

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
Actions #4

Updated by swildner almost 17 years ago

Uh, can you use diff -u please?

Sascha

Actions #5

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
Actions #6

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

Actions #7

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

Actions #8

Updated by c.turner almost 17 years ago

thanks - applied

Actions #9

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

Actions #11

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

Actions #12

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
Actions #13

Updated by hasso almost 17 years ago

I'd add -p to diff. It sometimes helps a lot.

Actions #14

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
Actions #15

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
&lt;&gt;
Actions #16

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

Actions #17

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
Actions #18

Updated by c.turner almost 17 years ago

woohoo! just saw the commits. thanks!

Now for that vktty thing.. (eventually)

- Chris

Actions

Also available in: Atom PDF