Messages in this thread | | | Date | Fri, 23 Oct 2009 20:45:48 -0700 | From | Darren Hart <> | Subject | Re: [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
| |