XATTR support on JFFS2 (version. 5)

Hello,

This attached patches provide xattr support including POSIX-ACL and SELinux support on JFFS2 (version.5).
We can apply those patches to the latest git-tree.
- xattr_on_jffs2.kernel.version-5.patch can be applied to mtd-2.6.git.
- xattr_on_jffs2.utils.version-5.patch can be applied to mtd-utils.git.

Thank you. These patches look good to me, in general. I do have a couple of minor comments to make, but I'll do that with a slightly shorter Cc list.

1. Please don't use typedefs. Just 'struct jffs2_acl_header' instead of this:

	typedef struct {
	       jint32_t        a_version;
	} jffs2_acl_header;

2. You're adding locks without documenting them in README.Locking. Please make sure any new locks (c->xattr_sem) are fully documented.

3. You have very strange locking behaviour in jffs2_garbage_collect_xattr() -- sometimes you return with the lock still held, sometimes without. I think it would be best to check ic->class within the jffs2_garbage_collect_passs() function instead.

4. You add a list_head to struct jffs2_inode_cache. That's two pointers -- could we get away with only a single pointer instead?

How much has your code been tested on SMP machines?

Please let me know if you'd like an account with direct access to the jffs2-xattr-2.6.git tree. I can continue to apply patches if you prefer, but git is quite easy to use.

OK, I'll fix it.

OK, I'll prepare it.
Please notice me, if storange English expressions are included.

Quoted 4 lines

3. You have very strange locking behaviour in jffs2_garbage_collect_xattr() -- sometimes you return with the lock still held, sometimes without. I think it would be best to check ic->class within the jffs2_garbage_collect_passs() function instead.

Do you prefer the following style implementation?
I also agree your opinion, if so. I'll fix it.

@ gc.c

    if (jffs2_garbage_collect_passs(c, ic)) {  /* returns 1, if ic is xdatum or xref */
        spin_unlock(&c->erase_completion_lock);
        jffs2_garbage_collect_xattr(c, ic);    /* ic is xdatum or xref */
        goto release_sem;
    }

Quoted 3 lines

4. You add a list_head to struct jffs2_inode_cache. That's two pointers -- could we get away with only a single pointer instead?

It's probably possible. Please wait for a while, since there is a bit much points to be fixed.

Quoted 1 line

How much has your code been tested on SMP machines?

It's not evaluated on SMP machine yet.
(And, I've not prepated SMP evaluation environment yet.) I hope to make collaboration with MTD people.

I haven't known much about detailed git operations yet. But I'm already doing basical operations such as pull/push/... in my local environment. Maybe, here is not significant problem. I'm willing to manage jffs2-xattr-2.6.git tree. Is it necessary to send you my ssh public key, isn't it?

I don't like that very much either, to be honest. How about getting rid of jffs2_garbage_collect_xattr() altogether, and using this in gc.c instead...

#ifdef CONFIG_JFFS2_FS_XATTR
	/* When 'ic' refers xattr_datum/xattr_ref, this node is GCed as xattr.
	   We can decide whether this node is inode or xattr by ic->class. */
	if (ic->class == RAWNODE_CLASS_XATTR_DATUM ||
	    ic->class == RAWNODE_CLASS_XATTR_REF) {
		spin_unlock(&c->erase_completion_lock);
	/* XXXXXX? */
		down_write(&c->xattr_sem);
		if (ic->class == RAWNODE_CLASS_XATTR_DATUM)
			ret = jffs2_garbage_collect_xattr_datum(c, (void *)ic);
		else
			ret = jffs2_garbage_collect_xattr_ref(c, (void *)ic);
		up_write(&c->xattr_sem);
		goto release_sem;
	}

#endif

Quoted 4 lines

I haven't known much about detailed git operations yet. But I'm already doing basical operations such as pull/push/... in my local environment. Maybe, here is not significant problem. I'm willing to manage jffs2-xattr-2.6.git tree. Is it necessary to send you my ssh public key, isn't it?

Yes, please. I'll add an account for you.

OK, I'll fix according to your suggestion.
Only one point, I think xattr_sem should be modified in xattr.c for readability.

Any SMP machine will work, using the 'mtdram' driver instead of real flash.

BTW, is xattr being friends with summary?

There are some significant differences from previous version posted at last December.
The biggest change is addition of EBS(Erase Block Summary) support.

Hello,

This attached patches provide xattr support including POSIX-ACL and SELinux support on JFFS2 (version.5).
We can apply those patches to the latest git-tree.
- xattr_on_jffs2.kernel.version-5.patch can be applied to mtd-2.6.git.
- xattr_on_jffs2.utils.version-5.patch can be applied to mtd-utils.git

+ * File: fs/jffs2/xattr.c

+#include <linux/xattr.h>

fs/jffs2/xattr.h +10 -0

+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 */

+struct jffs2_inode_cache; /* forward refence */

+extern struct jffs2_xattr_datum *jffs2_setup_xattr_datum(struct

+ uint32_t xid,

+extern void jffs2_xattr_free_inode(struct jffs2_sb_info *c, struct

Quoted 1 line

+#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?

Quoted 13 lines

fs/jffs2/xattr.h +13 -0

+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;
+

fs/jffs2/xattr.h line 38

Quoted 7 lines

fs/jffs2/xattr.h +7 -0

+	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.

Quoted 1 line

+struct jffs2_inode_cache; /* forward refence */

A classic example of a senseless comment :-)

fs/jffs2/xattr.h line 90

Quoted 2 lines

fs/jffs2/xattr.h +2 -0

+/*---- Any inline initialize functions ----*/
+#define init_xattr_inode_cache(x) INIT_LIST_HEAD(&((x)->ilist))

Wierd comment.

+/*-------------------------------------------------------------------------*

Quoted 1 line

+ * File: fs/jffs2/xattr.c

Not sure it makes sense to specify file name here.

Quoted 1 line

+#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?

Quoted 14 lines

fs/jffs2/xattr.h +14 -0

+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;

fs/jffs2/xattr.h line 38

Quoted 6 lines

fs/jffs2/xattr.h +6 -0

+	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.

Quoted 1 line

+struct jffs2_inode_cache; /* forward refence */

A classic example of a senseless comment :-)

fs/jffs2/xattr.h line 72

Quoted 7 lines

fs/jffs2/xattr.h +7 -0

+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.

fs/jffs2/xattr.h line 90

Quoted 2 lines

fs/jffs2/xattr.h +2 -0

+/*---- Any inline initialize functions ----*/
+#define init_xattr_inode_cache(x) INIT_LIST_HEAD(&((x)->ilist))

Wierd comment.

Quoted 3 lines · Artem B. Bityutskiy

+/*-------------------------------------------------------------------------*

Quoted 1 line

+ * 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.

Quoted 3 lines

Quoted 1 line

+#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".

Quoted 14 lines

Quoted 14 lines

fs/jffs2/xattr.h +14 -0

+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;

Quoted 8 lines

fs/jffs2/xattr.h line 38

Quoted 6 lines

fs/jffs2/xattr.h +6 -0

+	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.

Quoted 2 lines

Quoted 1 line

+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.

Quoted 10 lines

fs/jffs2/xattr.h line 72

Quoted 7 lines

fs/jffs2/xattr.h +7 -0

+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.

Quoted 3 lines

fs/jffs2/xattr.h line 90

Quoted 2 lines

fs/jffs2/xattr.h +2 -0

+/*---- 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 :)

fs/jffs2/acl.h +12 -0

+#ifdef __KERNEL__
+#ifdef CONFIG_JFFS2_FS_POSIX_ACL
+
+#define JFFS2_ACL_NOT_CACHED ((void *)-1)
+
+extern int jffs2_permission(struct inode *, int, struct nameidata *);
+extern int jffs2_acl_chmod(struct inode *);
+extern int jffs2_init_acl(struct inode *, struct inode *);
+extern void jffs2_clear_acl(struct inode *);
+
+extern struct xattr_handler jffs2_acl_access_xattr_handler;
+extern struct xattr_handler jffs2_acl_default_xattr_handler;

fs/jffs2/acl.h +2 -0

@@ -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;

[...]

fs/jffs2/acl.c line 52

Quoted 4 lines

fs/jffs2/acl.c +4 -0

+
+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.

fs/jffs2/acl.c line 61

Quoted 1 line

fs/jffs2/acl.c +1 -0

+	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.

fs/jffs2/acl.c line 67

Quoted 1 line

fs/jffs2/acl.c +1 -0

+	value = (char *)value + sizeof(jffs2_acl_header);

Can be removed.

fs/jffs2/acl.c line 79

Quoted 2 lines

fs/jffs2/acl.c +2 -0

+	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?

fs/jffs2/acl.c line 120

Quoted 1 line

fs/jffs2/acl.c +1 -0

+	jffs2_acl = (jffs2_acl_header *)kmalloc(sizeof(jffs2_acl_header)

kmalloc() returns a void*, so no explicit cast is needed.

fs/jffs2/acl.c line 149

Quoted 1 line

fs/jffs2/acl.c +1 -0

+	return (char *)jffs2_acl;

And this function returns void*, so this explicit cast isn't needed either.

#ifndef JFFS2_ACL_H
#define JFFS2_ACL_H
...
#endif

fs/jffs2/acl.h line 26

Quoted 2 lines

fs/jffs2/acl.h +2 -0

+#ifdef __KERNEL__
+#ifdef CONFIG_JFFS2_FS_POSIX_ACL

fs/jffs2/acl.h line 46

Quoted 2 lines

fs/jffs2/acl.h +2 -0

+#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.

Quoted 12 lines

fs/jffs2/nodelist.h +3 -1

@@ -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;

fs/jffs2/nodelist.h struct jffs2_inode_cache { · line 122

Quoted 4 lines

fs/jffs2/nodelist.h +3 -0

+#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.

[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.

Quoted 16 lines · KaiGai Kohei

[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

Quoted 2 lines

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

That's an excellent summary; thanks. It does serve to show that we've mostly picked on cosmetic details so far though :)

If you have patches committed into a git tree, you should be able to just 'git-push ssh://git.infradead.org/home/git/jffs2-xattr-2.6.git' to make it available.

Hi,

Some fixes and updates on the todo list are already pushed to git://git.infradead.org/jffs2-xattr-2.6.git .

The broken-out patches are available at here:
http://www.kaigai.gr.jp/index.php?FrontPage#vd169842

01. jffs2-xattr-v5.1-01-remove_typedef_kernel.patch
02. jffs2-xattr-v5.1-02-remove_typedef_utils.patch
03. jffs2-xattr-v5.1-03-append_README.Locking.patch
04. jffs2-xattr-v5.1-04-remove_ilist_from_ic.patch
05. jffs2-xattr-v5.1-05-update_xattr_gc.patch
06. jffs2-xattr-v5.1-06-add_list.h.patch
07. jffs2-xattr-v5.1-07-unify_file_header.patch
08. jffs2-xattr-v5.1-08-remove_senseless_comment.patch
09. jffs2-xattr-v5.1-09-remove__KERNEL__.patch
10. jffs2-xattr-v5.1-10-remove_pointer_cast.patch

I hope to merge the second patch (jffs2-xattr-v5.1-02-remove_typedef_utils.patch), because this patch is for mtd-utils.git, not jffs2-xattr-2.6.git.

P.S.
Thanks David for your introduction about git on infradead.org.

Quoted 29 lines · David Woodhouse

Quoted 16 lines · KaiGai Kohei

[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

Quoted 2 lines

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

That's an excellent summary; thanks. It does serve to show that we've mostly picked on cosmetic details so far though :)

If you have patches committed into a git tree, you should be able to just 'git-push ssh://git.infradead.org/home/git/jffs2-xattr-2.6.git' to make it available.

The pre-patched implementation ignores this error code and doesn't care. Theredore, total process of FS-mounting failed. This patch fixes this problem, When we found such a obsolete node, it will be dealt as a DIRTY_SPACE() and FS-mounting will be continued.

It works correctly, and I belive this solution is right.

It does look correct to me. The patch seems to highlight some inconsistency of tabs vs. spaces in the indenting though.

When committing to the a git tree, please remember that if I pull from it and then Linux pulls from it, the 'subject' of your commit should be meaningful in that context. So it should mention 'JFFS2' at the very least.

That's probably not quite so important for your jffs2-xattr tree, since I think I'll probably import that as a _patch_ when we're ready, rather than just pulling the whole thing. But in general, it's a useful thing to remember for people committing to git trees and expecting me to pull from them.

OK, I'll pay attention to attach 'JFFS2' to the subject. But nine of previous patches which have been already commited did not attach such a keyword. Is it necessary to update the subject of them ? (But I don't know how to operate for this. Hmm...)

Sorry, it's translated by terminal emulator automatically. The patch keeps consistency of tabs in the indenting.

OK, I'll pay attention to attach 'JFFS2' to the subject. But nine of previous patches which have been already commited did not attach such a keyword. Is it necessary to update the subject of them ? (But I don't know how to operate for this. Hmm...)

Alternatively, it's easy enough to cherry-pick patches from git and apply them to a new branch, then make that new branch your 'master'. If you do this, remember to push with the '--force' option so that your new tree overwrites the remote.

The all of previous 12 patches has '[JFFS2][XATTR]' prefix now. Please confirm at http://git.infradead.org/?p=jffs2-xattr-2.6.git;a=log

Quoted 16 lines · KaiGai Kohei

[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

Quoted 2 lines

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

That's an excellent summary; thanks. It does serve to show that we've mostly picked on cosmetic details so far though :)

This attached patches provide xattr support including POSIX-ACL and SELinux support on JFFS2 (version.5).
We can apply those patches to the latest git-tree.
- xattr_on_jffs2.kernel.version-5.patch can be applied to mtd-2.6.git.
- xattr_on_jffs2.utils.version-5.patch can be applied to mtd-utils.git.

Few suggestions for now (again cosmetic):

1. GC-related functions and summary-related functions should be moved to gc.c and summary.c

2. JFFS2 uses namings we all are used to. E.g., c - JFFS2 superblock structure, ref - struct jffs2_node_ref. I've noticed you use 'ref' as jffs2_xattr_ref pointers which is just a bit confusing. I would suggest you calling them 'xref' - this would improve readability a bit.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		delete_xattr_ref(c, ref);
+		delete_xattr_ref(c, xref);

It's minor, but still.

Will you object if I just do some changes I consider useful in xattr GIT? Or should I first mail you?

It's not even 'minor'. It's total bullshit. If you can't look at the function name "delete_XATTR_REF" and work out what 'ref' is in that context, then you really shouldn't be trying to read the code. Life is hard; let's go shopping.

I got annoyed with you when you started on stuff like this in JFFS2 code. Don't start on the XATTR code too.

I do see your point to a _certain_ extent, but let's concentrate on things that actually matter instead, OK?

Quoted 4 lines

Few suggestions for now (again cosmetic):

1. GC-related functions and summary-related functions should be moved to gc.c and summary.c

Then they'd need ifdefs. I'd prefer to keep them where they are, in separate files which aren't compiled if the XATTR options are disabled.

Quoted 4 lines

2. JFFS2 uses namings we all are used to. E.g., c - JFFS2 superblock structure, ref - struct jffs2_node_ref. I've noticed you use 'ref' as jffs2_xattr_ref pointers which is just a bit confusing. I would suggest you calling them 'xref' - this would improve readability a bit.

Really, I don't think it matters.

Quoted 4 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+		delete_xattr_ref(c, ref);
+		delete_xattr_ref(c, xref);

It's minor, but still.

It's not even 'minor'. It's total bullshit. If you can't look at the function name "delete_XATTR_REF" and work out what 'ref' is in that context, then you really shouldn't be trying to read the code. Life is hard; let's go shopping.