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

KaiGai Kohei
2006-05-08T02:04:33Z

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>