Re: NGROUPS 2.6.2rc2
From: Robin Holt
Date: Wed Jan 28 2004 - 06:34:01 EST
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/