Re: [PATCH] XATTR support on JFFS2 (version. 5)

[KaiGai Kohei <kaigai@ak.jp.nec.com>]

Hi, Jörn. Thanks for your comments.

> On Sat, 6 May 2006 15:51:22 +0900, KaiGai Kohei wrote:
>> +
>> +static struct posix_acl *jffs2_acl_from_medium(const void *value, size_t size)
>> +{
>> +	const char *end = (char *)value + size;
> 
> This cast appears to be unnecessary.  Gcc has an extention that allows
> arithmetic on void* pointers.  The behaviour is just as if the pointer
> was casted to unsigned long and back - quite useful.

Hmm, I'll review the implementation and reduce the unnecessary casts.


>> +	ver = je32_to_cpu(((jffs2_acl_header *)value)->a_version);
> 
> In this case, it might make sense to create a local variable:
> 	jffs2_acl_header *header = value;
> 	...
> 	ver = je32_to_cpu(header->a_version);
> 
> If only a single field is used from the header, this is a matter of
> taste.  If it occurs more often, the local variable helps with
> readability.

The most of posix-acl implementation on jffs2 is mede with referring
to ext2/3's implementation. The jffs2_acl_header structure is reflection
of ext2_acl_header.
I also think using local variable is good idea for readability. I'll fix it.


>> +	value = (char *)value + sizeof(jffs2_acl_header);
> 
> Can be removed.
> 
>> +	for (i=0; i < count; i++) {
>> +		jffs2_acl_entry *entry = (jffs2_acl_entry *)value;
> 
> This cast can be removed.  void* can implicitly be cast to any pointer
> type.  Also, would it be possible to move this variable to the
> beginning of the function and use it for the version check?

jffs2_acl_header is used for version checking, not jffs2_acl_entry.
Thus, it's not possible to use version checking. But I'll remove
unnecessary cast.

>> +	jffs2_acl = (jffs2_acl_header *)kmalloc(sizeof(jffs2_acl_header)
> 
> kmalloc() returns a void*, so no explicit cast is needed.
> 
>> +	return (char *)jffs2_acl;
> 
> And this function returns void*, so this explicit cast isn't needed
> either.

I agreed. Those should be reduced.

> #ifndef JFFS2_ACL_H
> #define JFFS2_ACL_H
> ...
> #endif
> 
>> +#ifdef __KERNEL__
>> +#ifdef CONFIG_JFFS2_FS_POSIX_ACL
   ... <snip> ...
>> +#endif	/* CONFIG_JFFS2_FS_POSIX_ACL */
>> +#endif	/* __KERNEL__ */
> 
> The kernel is in a fairly bad shape wrt. seperating userspace
> interface and in-kernel headers.  dwmw2 is currently trying to sort
> things out.  Splitting this part into a seperate file would make his
> job a bit easier.

Because 'acl.h' was shared by mkfs.jffs2 at previous version of xattr
patches. But the __KERNEL__ ifdef block is unnecessary now.
I'll remove it.

>> @@ -107,11 +109,16 @@ struct jffs2_inode_cache {
>>  		temporary lists of dirents, and later must be set to
>>  		NULL to mark the end of the raw_node_ref->next_in_ino
>>  		chain. */
>> +	u8 class;	/* It's used for identification */
>> +	u8 flags;
>> +	uint16_t state;
>>  	struct jffs2_inode_cache *next;
>>  	struct jffs2_raw_node_ref *nodes;
>>  	uint32_t ino;
>>  	int nlink;
>> -	int state;
>> +#ifdef CONFIG_JFFS2_FS_XATTR
>> +	struct list_head ilist;
>> +#endif
>>  };
> 
> Why does this field depend on CONFIG_JFFS2_FS_XATTR, while the above
> doesn't?  It looks as if splitting state into several fields is a
> generic cleanup and can be merged independently of xattr support.

If ilist would be always enabled, it will enforce any users the amount
of 2 * sizeof(void *) bytes memory usage increase per inode_cache
whether they hope to use xattr or not regardless of.
Thus, this field is defined with depending on CONFIG_JFFS2_FS_XATTR.

About the state variable, I splited it into three variable unconditionaly
because it didn't have such a bad effect (if int has 32-bit width).

Thanks,

[Current TODO list]
* Fix the declaration of jffs2_acl_header and so on by using 'struct'
   instead of 'typedef' in kernel space.
- Fix the declaration of jffs2_acl_header and so on by using 'struct'
   instead of 'typedef' in user space header.
* Add documentation about xattr_sem into README.Locking.
* Call jffs2_garbage_collect_xattr_datum/ref() from gc.c directly
   instead of jffs2_garbage_collect_xattr()
- Use unidirection list beween inode_chache and xattr_ref, instead of
   list_head.
- Add '#include <linux/list.h>' into xattr.h.
- Unify each file header part with any jffs2 files.
- Remove a senseless comment. ("/* forward refence */")
- Remove unneccesary pointer casts.
- Remove unneccesary '#ifdef __KERNEL__' on acl.h

*: It has already prepared, but I've not publicated yet.
-: It has not prepated yet. I should work from now.

-- 
Open Source Software Promotion Center, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>