On Fri, 2012-11-02 at 19:11 +0100, Jakub Hrozek wrote:
On Fri, Nov 02, 2012 at 01:53:52PM -0400, Simo Sorce wrote:
> On Thu, 2012-11-01 at 17:36 +0100, Pavel Březina wrote:
> > + if (ctx->is_daemon && ctx->parent_pid > 0
> > + && ctx->parent_pid == getppid()) {
> > + if (ctx->parent_pid <= 0 || ctx->parent_pid !=
getppid())
> > {
> > + /* the parent process was already terminated */
> > + DEBUG(SSSDBG_MINOR_FAILURE, ("Invalid parent pid
> > \n"));
> > + goto done;
> > + }
> > +
>
> Isn't the second if condition redundant here ?
> we can't even test it unless it is already true according to the
> previous if () or am I missing something ?
> Did you intend to use some || in the previous condition ?
>
Oops, I think you're right, thank you Simo.
To reiterate the discussion we had with Pavel in person[1], we wanted to
make sure we're really signaling the original parent and not e.g. init
in the odd case the parent died before we got to this point.
init has pid 1 iirc, should you check for > 1 ?
[1] This is a little off-topic, but as many of the SSSD developers
are now in
the same office, it is very convenient for us to talk in person. Obviously
this has the drawback of excluding the other developers and the community
from the discussion. We should try harder to document the thoughts in more
detail even though it might seem redundant.
Yes please, those comments should go either in the commit message or
comments in the code, or if they are just additional considerations in
the mail message that comes with the patch.
Simo.
--
Simo Sorce * Red Hat, Inc * New York