> -----Ursprüngliche Nachricht-----
> Von: Joe Orton
> Gesendet: Donnerstag, 28. Juni 2007 13:56
> An: dev@httpd.apache.org
> Betreff: Re: [PATCH] pid safety checks for 2.2.x
>
>
> On Wed, Jun 27, 2007 at 09:38:10PM +0200, Ruediger Pluem wrote:
> > > + /* Ensure the given pid is greater than zero;
> passing waitpid() a
> > > + * zero or negative pid has different semantics. */
> >
> > Ok, it seems as I am trying to become the king of all
> nitpickers :-):
> >
> > Style of comment.
>
> Happy to oblige but I'm not sure what the nit is - just my
> poor grammar
> or the slightly inappropriate reference to waitpid()?
It is simpler and more formal :-). My point was that it should be
/*
* Ensure the given pid is greater than zero; passing waitpid() a
* zero or negative pid has different semantics.
*/
instead of
/* Ensure the given pid is greater than zero; passing waitpid() a
* zero or negative pid has different semantics. */
But now I have to admit that there is no rule in our style guide for multiline
comments (I thought there was) and the style I proposed just seem to be used
very often. So sorry for nitpicking on this.
>
> > > + waitret = apr_proc_wait(&proc, NULL, NULL, APR_NOWAIT);
> > > + if (waitret == APR_CHILD_NOTDONE) {
> > > + return kill(pid, sig) ? errno : APR_SUCCESS;
> > > + }
> > > + else {
> > > + return APR_EINVAL;
> >
> > Hm. Wouldn't it make sense to log this in the case
> >
> > waitret != APR_CHILD_DONE
> >
> > as in the PID table patches?
> > This could give the admin a hint that something is rotten
> on his box.
>
> I guess this is worthwhile.
>
> The problem is that waitpid() does not distinguish between "child
> already reaped" (ignorable error) and "child not in process group"
> (something bad) so that will mean some unnecessary log spam in some
> cases.
I guess we are talking about ECHILD as the corresponding error value.
But under "non malicious child conditions" is there really the possibility
that we get here when the child was already reaped?
>
> The log spam is avoidable using getpgid() so I've resurrected that on
> platforms where available: (which will be 99%)
>
> So, final comments on this? If there's consensus that this is the
> approach to take I'll revert the pidtable stuff out of trunk, commit
> this there, and propose the backport.
I assume that your latest patch only differs from the previous one
in the implementation of ap_mpm_safe_kill and the additional line in
configure.in,
right?
Regards
Rüdiger
