On 10/27, Roland McGrath wrote:
>
> > --- /dev/null 2009-10-25 19:46:00.608018007 +0100
> > +++ V1/kernel/ptrace-utrace.c 2009-10-26 03:56:46.000000000 +0100
>
> Generally, needs more comments. That's not news, I'm sure.
> But just giving reactions as I would if doing upstream review
> without other context.
>
> > +struct ptrace_context {
> > + int options;
> > +
> > + int signr;
> > + siginfo_t *siginfo;
> > +
> > + int stop_code;
> > + unsigned long eventmsg;
> > +
> > + enum utrace_resume_action resume;
> > +};
>
> If you are lining up whitespace in field decls, do them all. This struct
> merits a top comment about what it is and where it hangs. Either in more
> top comment or interspersed, at least a little on what the fields are for.
Oh, comments. Yes we need more. I can only say I will try to add
more documentation, but this is not easy.
> > +static inline bool ptrace_event_pending(struct ptrace_context *context)
>
> s/context/ctx/ throughout for variable names (but leave ptrace_context as
> the struct name).
OK.
>
> > + if (action != UTRACE_REPORT)
> > + ptrace_context(engine)->stop_code = 0;
> > + utrace_control(tracee, engine, action);
>
> __must_check
OK.
> > +/**
> > + * ptrace_traceme -- helper for PTRACE_TRACEME
>
> Don't use /** kerneldoc magic. You're not matching the whole form,
> and these are not functions we extract docs for anyway.
Heh, this was blindly copied from the old code, will remove that part.
> > +void ptrace_notify_stop(struct task_struct *tracee)
> > +{
> > + struct utrace_engine *engine = ptrace_lookup_engine(tracee);
> > +
> > + if (IS_ERR(engine)) {
> > + // XXX: temporary check, wrong with mutlitracing
> > + WARN_ON(tracee->state != TASK_RUNNING);
> > + return;
>
> Explain this scenario at least a little in the comment.
This WARN (and another "XXX" comment in ptrace_resume) should go away.
> > +#ifdef PTRACE_SINGLESTEP
> > + case PTRACE_SINGLESTEP:
> > +#endif
> > +#ifdef PTRACE_SINGLEBLOCK
> > + case PTRACE_SINGLEBLOCK:
> > +#endif
> > +#ifdef PTRACE_SYSEMU
> > + case PTRACE_SYSEMU:
> > + case PTRACE_SYSEMU_SINGLESTEP:
> > +#endif
> > + case PTRACE_SYSCALL:
> > + case PTRACE_CONT:
> > + ret = ptrace_resume(child, engine, request, data);
> > + break;
>
> Since ptrace_resume_action has a switch with these #ifdef's anyway,
> maybe make this the default case here and give that other switch a
> default error case.
Hmm. Yes, at first glance this would be cleaner...
> > + case PTRACE_KILL:
> > + ret = 0;
> > + if (!child->exit_state) /* already dead */
> > + ret = ptrace_resume(child, engine, request, SIGKILL);
> > + break;
>
> This could be inside ptrace_resume too.
Yes, and we don't even need to check exit_state.
> That fits better if you fold
> ptrace_resume_action into ptrace_resume, and I don't see any reason at
> all not to do that anyway.
Well, I'd like to keep ptrace_resume_action(). ptrace_resume() is fat
enough, and while ptrace_resume_action's code is simple, it is not easily
readable.
But yes, we can fold it although I don't understand why you don't like it,
to me its logic deserves a separate function. If nothing else, ptrace_resume()
becomes >100 lines after the folding.
Oleg.
