On Wed, Feb 07, 2007 at 10:04:26AM +0800, Ian jonhson wrote: Ian, you may wish to skim the kernel coding style guidelines; it'd make it easier for others to help review your code if the style is more what reviewers are used to.
Documentation/CodingStyle
> static int my_setprocattr(struct task_struct *p, char *name, void
> *value, size_t size)
> {
> my_struct sl;
> my_struct* ts;
>
> if(current != p)
> return -EACCES;
>
> if(!size)
> return -ERANGE;
>
> if(!strcmp(name, "current"))
> {
> if (copy_from_user(&sl, value, sizeof(my_struct))) {
> return -EFAULT;
> }
>
> ts = p->security;
I do hope that your module is allocating a structure for this at some
point. :)
> if(ts)
> {
> ts->v1 = sl.v1;
> ts->v2 = sl.v2;
I realize this is just testing code for now, but it seems unwise to
trust whatever these paramters mean completely.
>
> }
> else
> {
> ts->v1 = UNDEFINE_V1;
> ts->v2 = UNDEFINE_V2;
> }
If ts is NULL, it would not be wise to dereference member pointers. :)
>
> return 0;
> }
>
> return size;
> }
This one function returns -EACCES, -ERANGE, -EFAULT, 0, and whatever
'size' was. (But 'size' is never used as a size in this function.)
I'm not sure that you've explored all the code paths that could come
through this routine. :)
(Sorry, I don't know the setprocattr interface right off the top of my
head, but returning '0' for a successful write to "current" and size
for an unsuccessful write (anything other than "current") just doesn't
feel right.)
Hope this helps
pgpy3l5Ry8Sig.pgp
Description: PGP signature
