On Fri, Jul 31, 2009 at 09:59:11AM -0700, Jim Keniston wrote:
>
> On Thu, 2009-07-30 at 15:26 +0530, Ananth N Mavinakayanahalli wrote:
>
> > +static u32 core_clone(enum utrace_resume_action action,
> ...
> > + /*
> > + * -EINPROGRESS => thread on the way to quiesce
> > + * -EALREADY => we may be racing with attach_utrace_engines
> > + * and it may already have attached an engine for us.
> > + */
> > + if (ret && ((ret != -EINPROGRESS) || (ret != -EALREADY))) {
>
> ((ret != -EINPROGRESS) || (ret != -EALREADY)) ???
> Assuming EINPROGRESS != EALREADY, this test is always true. I think you
> want && here, in which case you can dispense with most of the parens.
Yup, you are right. I'll fix it.
> ...
> > +/*
> > + * Process exec()ing. Abort and exit from here
> > + *
> > + * XXX:
> > + * a. Is this necessary?
>
> I think it's a good idea to handle the exec case. But since dumping
> core for an exec-ing process is a rather bizarre thing to attempt, you
> can probably handle it however is most convenient for you.
Yes, in light of Roland's detailed description, all these will change
for sure.
> > + * b. Are we safe here wrt the core dump being in progress?
>
> Assuming all threads are quiesced before you start your actual core
> dump, there's no way one of them can exec while you're dumping.
Right.
> > + */
> > +static u32 core_exec(enum utrace_resume_action action,
> > + struct utrace_engine *engine, struct task_struct *task,
> > + const struct linux_binfmt *fmt, const struct linux_binprm *bprm,
> > + struct pt_regs *regs)
> > +...
> > +
> > +static struct core_proc *attach_utrace_engines(struct task_struct *task)
> > +{
> ...
> > + /*
> > + * -EINPROGRESS => we are on our way to quiesce.
> > + * -EEXIST => we may be racing with core_clone and it has
> > + * already attached an engine for us.
> > + */
> > + if (ret && ((ret != EINPROGRESS) || (ret != -EALREADY)))
>
> s/||/&&/ -- as above.
Agreed.
Ananth
