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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site