DragonFlyBSD

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

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

Uh, can you use diff -u please?

Sascha

This is the ~/.cvsrc file we recommend:

cvs -q
diff -u
update -Pd
checkout -P
rtag -a

-Matt

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

if (pidfile != NULL)

+ writepid(pidfile);
cpu_disable_intr();
init_sys_memory(memImageFile);
init_kern_memory();

... otherwise you get a segmenation fault.

Joerg

thanks - applied

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

Wiki updated to reflect this:
http://wiki.dragonflybsd.org/index.cgi/HandyHints#head-4d3ffe00189ec577f0ea51caa853e61bcb2962a7

Thanks,
Nuno

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

: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

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

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

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

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

: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

woohoo! just saw the commits. thanks!

Now for that vktty thing.. (eventually)

- Chris