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

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

Hi, David and Aterm.
Thanks for your comments.

David Woodhouse wrote:
> On Sun, 2006-05-07 at 16:46 +0400, Artem B. Bityutskiy wrote:
> 
>> +/*-------------------------------------------------------------------------*
>>> + *  File: fs/jffs2/xattr.c
>> Not sure it makes sense to specify file name here.
> 
> It's a common thing to do though; I don't think it matters much.
> 
> I would prefer to to have a consistent 'header' across all the JFFS2
> files though -- I would have been inclined to copy the form used in
> those. It's purely cosmetic though.

I agree. The file header part at xattr.[ch], acl.[ch], security.c,
xattr_user.c and xattr_trusted.c should be updated.

>>> +#include <linux/xattr.h>
>> You're using 'struct list_head' in your 'xattr.h' file, wouldn't it be a
>> good tone to add #include <linux/lists.h> then?
> 
> Yes, it's good practice to include the header files you need directly,
> rather than relying on the C file to include stuff like <linux/list.h>
> before including "xattr.h". 

OK, I'll fix it.

>>> +struct jffs2_xattr_datum
>>> +{
>>> +	void *always_null;
>>> +	u8 class;
>>> +	u8 flags;
>>> +	u16 xprefix;			/* see JFFS2_XATTR_PREFIX_* */
>>> +
>>> +	struct jffs2_raw_node_ref *node;
>>> +	struct list_head xindex;	/* chained from c->xattrindex[n] */
>>> +	uint32_t refcnt;		/* # of xattr_ref refers this */
>>> +	uint32_t xid;
>>> +	uint32_t version;
>>> +
>>> +	uint32_t data_crc;
>>> +	uint32_t hashkey;
>>> +	char *xname;		/* XATTR name without prefix */
>>> +	uint32_t name_len;	/* length of xname */
>>> +	char *xvalue;		/* XATTR value */
>>> +	uint32_t value_len;	/* length of xvalue */
>>> +};
> 
>> Would be cuter to use Linux-style comments.
> 
> I think Artem is referring to kernel-doc style comments, which look like
> this...
> 
> /**
>  * struct jffs2_xattr_datum - inode_cache equivalent for XATTR data
>  * 
>  * @always_null: Equivalent of ic->scan_dents. Must be NULL
>  * @class:       Used to distinguish from jffs2_inode_cache/jffs2_xattr_ref
> ...
> 
> I suppose it _might_ be nice if we were to do kerneldoc-style comments
> all through JFFS2, and create a proper set of documentation for the
> source code. I don't think it's fair to expert _you_ to start on that
> though; there's plenty of work for Artem to do first if he wants that. 
> 
> Personally, I prefer the comments _with_ the struct members, on the
> right-hand-side as you've placed them. I'd want to keep those even if we
> _do_ also have the kerneldoc comments which produce _external_
> documentation for printing.

I'll follow the common style on jffs2 implementation, when we can arrive
at an agreement. Now, I hope to suspend it.

>>> +struct jffs2_inode_cache;	/* forward refence */
>> A classic example of a senseless comment :-)
> 
> Yes, it's best to avoid commenting on things which don't need it.

I'll remove it.
(Futhermore, it's wrong in spelling. orz)

>>> +extern struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct jffs2_sb_info *c,
>>> +                                                  uint32_t xid, uint32_t version);
>>> +
>>> +extern void jffs2_xattr_delete_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *ic);
>>> +extern void jffs2_xattr_free_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *ic);
>>> +
>>> +extern int jffs2_garbage_collect_xattr(struct jffs2_sb_info *c, struct jffs2_inode_cache *ic);
> 
>> I wouldn't follow old JFFS2 style and would not exceed the 80-characters
>> per line limit.
> 
> Even in prototypes? I disagree. Limiting yourself to 80 characters just
> makes it messier.

I'll follow the common style on jffs2 implementation, when we can arrive
at an agreement. Now, I hope to suspend it.


>>> +/*---- Any inline initialize functions ----*/
>>> +#define init_xattr_inode_cache(x) INIT_LIST_HEAD(&((x)->ilist))
>> Wierd comment.
> 
> I think it was meant to read something like:
> 
>  /*----- inline initialisation functions -----*/
> 
> But yes, since there's only one 'function' defined there and it's fairly
> obvious that it's an initialisation function, the comment is probably
> not needed.
> 
> If all Artem can come up with is cosmetics, then I'm sure we're doing
> well :)

Ah, some inline functions were defined here at the previous versions of
xattr patches. This comment remained with old information.

Futhermore, init_xattr_inode_cache() will become also unnecesary because
'list_head ilist' will be replaced by 'struct jffs2_xattr_ref *xref' and
'xref' will be initialized as NULL by memset(ic, 0, sizeof(*ic)).
David suggested me to use single direction list instead of list_head for
memory usage reduction at previous reply.

Thus, I'll remove both this comment and definition of init_xattr_inode_cache().

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