[EMAIL PROTECTED] wrote on Tue, 20 Feb 2007 22:49 -0600:
> Some notes on the changes I made:
I'm glad you're doing this. It's a necessary evil. I think all the
caveats you listed are fine.
Some comments on the patch.
+#define PVFS_DEFAULT_SGIDS_COUNT 32
+
/** Credentials (stubbed for future authentication methods). */
typedef struct
{
PVFS_uid uid;
PVFS_gid gid;
+ PVFS_gid *s_gids; /* array of secondary gids */
+ int32_t s_gids_count; /* count of secondary gids */
+#if PVFS2_SIZEOF_VOIDP == 64
+ int32_t __pad1;
+#endif
+
} PVFS_credentials;
-endecode_fields_2(
+endecode_fields_2a(
PVFS_credentials,
PVFS_uid, uid,
- PVFS_gid, gid);
+ PVFS_gid, gid,
+ int32_t, s_gids_count,
+ PVFS_gid, s_gids);
We'll need a custom encoder for this now. You must make sure to put
an even number of 8-byte words in the request, else future parts of
the encoded buffer won't align properly. 1: uid+gid, 2: s_gids_count
+ s_gids[0], 3: s_gids[1] + s_gids[2], etc. I.e., add a u32 padding
if s_gids_count is even.
Are you sharing the credentials struct with the kernel? On a 64-bit
kernel with 32-bit userspace, pointers have different sizes. You
can test this on an x86_64 box, compiling the client code with -m32
to see what happens.
+ if(sgids_count > 0)
+ {
+ credentials->s_gids =
+ (PVFS_gid *)(credentials + sizeof(PVFS_credentials));
+ }
Addition is pointers is defined so:
struct whatever *w
int n;
w + n == &w[N] == (char *)w + n * sizeof(struct whatever)
You probably want:
credentials->s_gids = (void *) (credentials + 1);
or this, found further on in the patch:
+ ret->s_gids = (PVFS_gid *)(((char *)ret) +
sizeof(PVFS_credentials));
A run through valgrind might be worth it to see if there are other
gotchas with this variable length gids array now.
+ namegroup_buffer = malloc(group_buffer_size);
Can you not allocate this every trip through the loop? Just
once at the top like group_buffer.
Actually I'm not understanding what's going on. You lookup the gid
to find the list of _users_ in that gid. Then for each one of them,
you look up the group that happens to have the same name as that
user. Why not just call getgroups()?
- memcpy(ret, credentials, sizeof(PVFS_credentials));
You can still do memcpy, just use cred_size. No need to write
all the fields by hand.
+ info->size > (sizeof(PINT_dev_header_t) +
\
+ sizeof(pvfs2_upcall_t) +
\
+ (sizeof(PVFS_gid) *
\
+ info->credentials->s_gids_count)))
\
You'll want to make sure things are aligned here too, in case your
trailer struct includes any 64-bit objects. (And can you make this
a function? Gigantic #defines are icky.)
I didn't read all of devpvfs2-req.c. There's lots of changes there.
But barring bugs, I think the approach is fine.
-- Pete
_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
