lkml.org 
[lkml]   [2009]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH] Add prctl to set sibling thread names (take two)
Hi John,

Just a couple nitpics really, looks pretty good to me - other than the
need for the wmb()s below.

john stultz wrote:
> This patch exports a task's comm via proc/pid/comm and
> proc/pid/task/tid/comm interfaces, and allows thread siblings to write
> to these values.

And the parent I presume?

> + /*
> + * Threads may access current->comm without holding
> + * the task lock, so write the string carefully
> + * to avoid non-terminating reads. Readers without a lock
> + * with get the oldname, the newname or an empty string.

s/with/will/
s/oldname/old name/ (it isn't a variable right?)
s/newname/new name/ (it isn't a variable right?)

> + */
> + tsk->comm[0] = NULL;
> + /* XXX - Need an mb() here?*/

I believe you do, yes. Now, which one... hrm... checking... You only
care about ensuring the the comm[0] store occurs BEFORE the strlcpy.
But, if no lock is held here, you can be preempted, so this is important
for both UP and SMP. I believe what you need here is:

wmb()

Memory barrier experts, please enlighten us if I am missing something.

> + strlcpy(tsk->comm+1, buf+1, sizeof(tsk->comm)-1);

And one more here I should think, otherwise that could effectively undo
the previous one :-)

wmb()

> + tsk->comm[0] = buf[0];
> task_unlock(tsk);

To be clear, we hold the lock to prevent other threads from changing
this at the same time as us - any other thread but the target thread
that is?

> +static ssize_t
> +comm_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *offset)
> +{
> + struct inode *inode = file->f_path.dentry->d_inode;
> + struct task_struct *p;
> + char buffer[TASK_COMM_LEN];
> +
> + memset(buffer, 0, sizeof(buffer));

What purpose does zeroing this entire buffer serve?

> + if (count > sizeof(buffer) - 1)
> + count = sizeof(buffer) - 1;
> + if (copy_from_user(buffer, buf, count))
> + return -EFAULT;
> +

Extra whitespace

> +
> + p = get_proc_task(inode);
> + if (!p)
> + return -ESRCH;
> +
> + if (same_thread_group(current, p))
> + set_task_comm(p, buffer);
> + else
> + count = -EINVAL;
> +
> + put_task_struct(p);
> +
> + return count;
> +}

Thanks,

--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team


\
 
 \ /
  Last update: 2009-10-24 05:49    [from the cache]
©2003-2011 Jasper Spaans