svn commit: r270444 - in head/sys: kern sys
mjguzik at gmail.com
Thu Aug 28 08:47:16 UTC 2014
On Thu, Aug 28, 2014 at 11:21:39AM +0300, Konstantin Belousov wrote:
> On Thu, Aug 28, 2014 at 05:30:09AM +0200, Mateusz Guzik wrote:
> > @@ -791,6 +791,8 @@ fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp)
> > struct ucred *cred;
> > struct sigacts *ps;
> > + /* For proc_realparent. */
> > + sx_assert(&proctree_lock, SX_LOCKED);
> > PROC_LOCK_ASSERT(p, MA_OWNED);
> > bzero(kp, sizeof(*kp));
> > @@ -920,7 +922,9 @@ fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp)
> > kp->ki_acflag = p->p_acflag;
> > kp->ki_lock = p->p_lock;
> > if (p->p_pptr)
> > - kp->ki_ppid = p->p_pptr->p_pid;
> > + kp->ki_ppid = proc_realparent(p)->p_pid;
> Is the check for p_pptr != NULL still needed for the call to
> proc_realparent() ? If yes, I think it indicates a bug in
> proc_realparent(), which should incorporate the check, instead of
> enforcing it on the callers. It seems to be there for the kernel process
> (pid 0).
As it is proc_realparent cannot fail, so after this change callers like
this one would have to have some checks anyway. On the other hand other
consumers don't need to worry about this edge case, so it would only add
an unnecessary branch.
I have no strong opinion either way, for now I decided to just commit
the patch in its current form.
> If the test can be removed, and proc_realparent() called unconditionally,
> I suggest to remove assert about proctree_lock at the start of
> fill_kinfo_proc_only(), since the check is done in proc_realparent().
> Whatever decision is made there, it can be implemented after your
> change is landed. The patch looks fine for me.
Thanks, committed as r270745.
Mateusz Guzik <mjguzik gmail.com>
More information about the svn-src-all