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    [W:0.023 / U:60.072 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site