Re: [PATCH]shm to sysfs ready (?)

[Patrick Mochel <mochel@osdl.org>]

>   Summary :
> 
>       -shm destruction moved to kobject release function
>       -id seek was trivial.Attrshow rewritten.
>       -Tested against recursive fgreps-kde-personal benchmark program
>       -Patched might_sleep problem
>       -Checking release code in front of all shm_destroy calls
>       -No more MAJ charset for attributes
>       -Adding EOL to attr values.
>       -Working against 2.6.0.test2
> 
>       Is it ok for you ?

A few comments below. 

> +#include <linux/kobj_map.h>

You shouldn't need to include this. 

> +	strcpy(shm_ids.kobj.name, "shm");
> +	shm_ids.kobj.parent = NULL;
> +	shm_ids.kobj.kset = NULL;
> +	shm_ids.kobj.ktype = NULL;
> +	kobject_register(&shm_ids.kobj);

The kobject internals rely on the entire kobject being zero-filled. You 
should be memset'ing it (and probably the entire object) earlier, which 
saves you from manually initializing the fields above to 0, and prevents 
you from missing any in the future. 

> +static ssize_t shm_attr_show(struct kobject *kobj, struct attribute *attr, char *buf)
> +{
> +	unsigned int id=simple_strtoul(kobj->name,NULL,10)%SEQ_MULTIPLIER;
> +	struct shmid_kernel *shp;
> +	down (&shm_ids.sem);
> +	shp=shm_lock(id);
> +	if(!strcmp(attr->name,"key"))
> +		snprintf(buf, PAGE_SIZE,"%ld\n", (long)shp->shm_perm.key);
	...

If you're going to do it this way, I suggest declaring the attributes 
before the show method, so you can do a pointer comparison, rather than a 
string compare: 

+	if(attr == &shm_attr_key))
+		snprintf(buf, PAGE_SIZE,"%ld\n", (long)shp->shm_perm.key);
	...

> +#define SHM_ATTR(_name) \
> +static struct attribute shm_attr_##_name={ \
> +	.name=__stringify(_name), \
> +	.mode=0444, \
> +}; \

Also, you should declare all the attributes here, rather than in the 
registration function. They will only be defined once, and will not put 
unnecessary pressure on the stack. 

struct kobj_type::default_attrs is designed to hold an array of attributes
that are added to each kobject registered of that type. This is done
automatically for you, so you don't have to do it manually. (It also
checks the errors as it adds each attribute). I suggest using that. 

Thanks,


	-pat