On 10/27, Roland McGrath wrote:
>
> I'm skeptical this is the desireable way to move the code around.
> Of course, for all such things, I am fine with whatever upstream likes.
> But here are my concerns:
I am not sure this is the best choice too.
> That is not friendly to git history at all.
Yes, I agree completely.
> If you move big chunks of code
> to different files, it's ideal to do it in a sequence of changes wherein
> the git file rename detection will grok what you are doing.
Not sure I understand. Do you mean it is possible to move the code from
the old file to the new one in a git-friendly manner? Afaics, there is no
way to do this, git can only hanlde renames. (but my git skills is close
to zero).
> I don't see how this route is any better than having the new and old code
> in kernel/ptrace.c with #ifdef.
This is what I'd like to avoid as much as possible. As usual, this is very
subjective, but imho this way it will be very difficult to read the code,
even if we have a single ifdef which separates the old and new implementation.
Say, not good to have 2 almost identical ptrace_attach() implementations in
the same file.
Instead of the single ifdef, we can add a lot of them. For example, as for
ptrace_attach() we can do
int ptrace_attach(struct task_struct *task)
{
int retval;
audit_ptrace(task);
retval = -EPERM;
if (unlikely(task->flags & PF_KTHREAD))
goto out;
if (same_thread_group(task, current))
goto out;
/*
* Protect exec's credential calculations against our
interference;
* interference; SUID, SGID and LSM creds get determined
differently
* under ptrace.
*/
retval = -ERESTARTNOINTR;
if (mutex_lock_interruptible(&task->cred_guard_mutex))
goto out;
task_lock(task);
retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
task_unlock(task);
if (retval)
goto unlock_creds;
#ifdef CONFIG_UTRACE
retval = ptrace_attach_task(task, 0);
if (unlikely(retval))
goto unlock_creds;
#endif
write_lock_irq(&tasklist_lock);
retval = -EPERM;
if (unlikely(task->exit_state))
goto unlock_tasklist;
#ifdef CONFIG_UTRACE
BUG_ON(task->ptrace);
task->ptrace = PT_UTRACED;
#else
if (task->ptrace)
goto unlock_tasklist;
task->ptrace = PT_PTRACED;
#endif
if (capable(CAP_SYS_PTRACE))
task->ptrace |= PT_PTRACE_CAP;
__ptrace_link(task, current);
send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
retval = 0;
unlock_tasklist:
write_unlock_irq(&tasklist_lock);
unlock_creds:
mutex_unlock(&task->cred_guard_mutex);
out:
return retval;
}
but this is much, much worse and guess you didn't mean this.
> Conversely, just leave the common code in
> ptrace.c and compile it there. AFAICT the only static function that would
> need to be made global for that is __ptrace_detach.
OK, agreed, this can work too.
In this case kernelptrace.c becomes
... the context of kernel/ptrace-common.h ...
#ifndef CONFIG_UTRACE
... other code ...
#endif
and we don't need CONFIG_PTRACE_OLD.
Do you agree with approach?
Oleg.
