> --- /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.
> +#define PT_UTRACED 0x00001000
> +
> +#define PTRACE_O_SYSEMU 0x100
> +
> +#define PTRACE_EVENT_SYSCALL_ENTRY (1 << 16)
> +#define PTRACE_EVENT_SYSCALL_EXIT (2 << 16)
> +#define PTRACE_EVENT_SIGTRAP (3 << 16)
> +#define PTRACE_EVENT_SIGNAL (4 << 16)
> +/* events visible to user-space */
> +#define PTRACE_EVENT_MASK 0xFFFF
Needs comments about what it is, and why it is not in linux/ptrace.h.
Personally I am glad to have these private bits in private source only.
But it certainly looks to the casual observer like these belong next to
the macros for the adjacent bits.
> +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).
> + if (action != UTRACE_REPORT)
> + ptrace_context(engine)->stop_code = 0;
> + utrace_control(tracee, engine, action);
__must_check
> +/**
> + * 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.
> +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.
> +#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.
> + 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. 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.
Thanks,
Roland
