lkml.org 
[lkml]   [2004]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: NGROUPS 2.6.2rc2
    On Tue, Jan 27, 2004 at 02:53:11PM -0800, Tim Hockin wrote:

    For the every declaration of struct group_info *info, is there a
    more descriptive name than info available. This will make
    searchs simpler in the long run.


    +#define NGROUPS_SMALL 32
    +#define NGROUPS_BLOCK ((int)(EXEC_PAGESIZE / sizeof(gid_t)))

    Can this be changed to NGROUPS_PER_EPAGE. NGROUPS_BLOCK seems to
    indicate a location of a block or the block or some block given an offset.
    NGROUPS_PER_EPAGE is very explicit.

    +void groups_free(struct group_info *info)
    {
    + if (info->ngroups > NGROUPS_SMALL) {

    Why not use info->nblocks > 0. These are more clearly tied to each
    other than ngroups and NGROUPS_SMALL.


    + while (left >= 0 && GROUP_AT(info, left) > tmp) {
    + GROUP_AT(info, right) = GROUP_AT(info, left);
    + left -= stride;
    + right = left + stride;

    For the above in groups_sort, why not just have
    right = stride;
    left -= stride;



    @@ -1102,54 +1256,43 @@

    if (gidsetsize < 0)
    return -EINVAL;
    - i = current->ngroups;
    + i = current->group_info->ngroups;
    if (gidsetsize) {
    if (i > gidsetsize)
    return -EINVAL;
    - if (copy_to_user(grouplist, current->groups, sizeof(gid_t)*i))
    + if (groups_to_user(grouplist, current->group_info))
    return -EFAULT;
    }

    Shouldn't there be a get/put pair around this. Especially with the
    copy_to_user call buried in groups_to_user, we could spend quite some time
    waiting for the page fault and another thread could free our structure.
    Or maybe I am missing something...


    + if (info->ngroups > TASK_SIZE/sizeof(group))
    + return -EFAULT;

    I don't understand the TASK_SIZE usage. Maybe this is a difference
    between ia64 and i386, but these checks don't make any sense to
    me. Consider them not looked at.


    asmlinkage long sys_getgroups16(int gidsetsize, old_gid_t __user *grouplist)
    {
    - old_gid_t groups[NGROUPS];
    - int i,j;
    + int i = 0;

    if (gidsetsize < 0)
    return -EINVAL;
    - i = current->ngroups;
    + i = current->group_info->ngroups;
    if (gidsetsize) {
    if (i > gidsetsize)
    return -EINVAL;
    - for(j=0;j<i;j++)
    - groups[j] = current->groups[j];
    - if (copy_to_user(grouplist, groups, sizeof(old_gid_t)*i))
    + if (groups16_to_user(grouplist, current->group_info))

    Again with the get/put.

    ===== fs/proc/array.c 1.55 vs edited =====
    --- 1.55/fs/proc/array.c Tue Oct 14 14:00:09 2003
    +++ edited/fs/proc/array.c Tue Jan 27 12:40:02 2004
    @@ -176,8 +176,8 @@
    p->files ? p->files->max_fds : 0);
    task_unlock(p);

    - for (g = 0; g < p->ngroups; g++)
    - buffer += sprintf(buffer, "%d ", p->groups[g]);
    + for (g = 0; g < min(p->group_info->ngroups,NGROUPS_SMALL); g++)
    + buffer += sprintf(buffer, "%d ", GROUP_AT(p->group_info,g));

    buffer += sprintf(buffer, "\n");
    return buffer;

    Again with the get/put.

    All of the sunrpc mods appear to need get/put.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 14:00    [W:0.026 / U:0.156 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site