Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


mathieu.segaud at regala

Jan 4, 2008, 5:21 AM

Post #1 of 13 (299 views)
Permalink
[PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl

Signed-off-by: Mathieu Segaud <mathieu.segaud[at]regala.cx>
---
fs/ext3/acl.c | 194 ++++++++++++++++++++++++++++----------------------------
fs/ext3/acl.h | 6 +-
fs/ext4/acl.c | 190 ++++++++++++++++++++++++++++----------------------------
fs/ext4/acl.h | 6 +-
4 files changed, 198 insertions(+), 198 deletions(-)

diff --git a/fs/ext3/acl.c b/fs/ext3/acl.c
index d34e996..8de0050 100644
--- a/fs/ext3/acl.c
+++ b/fs/ext3/acl.c
@@ -40,34 +40,34 @@ ext3_acl_from_disk(const void *value, size_t size)
acl = posix_acl_alloc(count, GFP_KERNEL);
if (!acl)
return ERR_PTR(-ENOMEM);
- for (n=0; n < count; n++) {
+ for (n = 0; n < count; n++) {
ext3_acl_entry *entry =
(ext3_acl_entry *)value;
if ((char *)value + sizeof(ext3_acl_entry_short) > end)
goto fail;
acl->a_entries[n].e_tag = le16_to_cpu(entry->e_tag);
acl->a_entries[n].e_perm = le16_to_cpu(entry->e_perm);
- switch(acl->a_entries[n].e_tag) {
- case ACL_USER_OBJ:
- case ACL_GROUP_OBJ:
- case ACL_MASK:
- case ACL_OTHER:
- value = (char *)value +
- sizeof(ext3_acl_entry_short);
- acl->a_entries[n].e_id = ACL_UNDEFINED_ID;
- break;
-
- case ACL_USER:
- case ACL_GROUP:
- value = (char *)value + sizeof(ext3_acl_entry);
- if ((char *)value > end)
- goto fail;
- acl->a_entries[n].e_id =
- le32_to_cpu(entry->e_id);
- break;
-
- default:
+ switch (acl->a_entries[n].e_tag) {
+ case ACL_USER_OBJ:
+ case ACL_GROUP_OBJ:
+ case ACL_MASK:
+ case ACL_OTHER:
+ value = (char *)value +
+ sizeof(ext3_acl_entry_short);
+ acl->a_entries[n].e_id = ACL_UNDEFINED_ID;
+ break;
+
+ case ACL_USER:
+ case ACL_GROUP:
+ value = (char *)value + sizeof(ext3_acl_entry);
+ if ((char *)value > end)
goto fail;
+ acl->a_entries[n].e_id =
+ le32_to_cpu(entry->e_id);
+ break;
+
+ default:
+ goto fail;
}
}
if (value != end)
@@ -96,27 +96,27 @@ ext3_acl_to_disk(const struct posix_acl *acl, size_t *size)
return ERR_PTR(-ENOMEM);
ext_acl->a_version = cpu_to_le32(EXT3_ACL_VERSION);
e = (char *)ext_acl + sizeof(ext3_acl_header);
- for (n=0; n < acl->a_count; n++) {
+ for (n = 0; n < acl->a_count; n++) {
ext3_acl_entry *entry = (ext3_acl_entry *)e;
entry->e_tag = cpu_to_le16(acl->a_entries[n].e_tag);
entry->e_perm = cpu_to_le16(acl->a_entries[n].e_perm);
- switch(acl->a_entries[n].e_tag) {
- case ACL_USER:
- case ACL_GROUP:
- entry->e_id =
- cpu_to_le32(acl->a_entries[n].e_id);
- e += sizeof(ext3_acl_entry);
- break;
-
- case ACL_USER_OBJ:
- case ACL_GROUP_OBJ:
- case ACL_MASK:
- case ACL_OTHER:
- e += sizeof(ext3_acl_entry_short);
- break;
-
- default:
- goto fail;
+ switch (acl->a_entries[n].e_tag) {
+ case ACL_USER:
+ case ACL_GROUP:
+ entry->e_id =
+ cpu_to_le32(acl->a_entries[n].e_id);
+ e += sizeof(ext3_acl_entry);
+ break;
+
+ case ACL_USER_OBJ:
+ case ACL_GROUP_OBJ:
+ case ACL_MASK:
+ case ACL_OTHER:
+ e += sizeof(ext3_acl_entry_short);
+ break;
+
+ default:
+ goto fail;
}
}
return (char *)ext_acl;
@@ -141,7 +141,7 @@ ext3_iget_acl(struct inode *inode, struct posix_acl **i_acl)

static inline void
ext3_iset_acl(struct inode *inode, struct posix_acl **i_acl,
- struct posix_acl *acl)
+ struct posix_acl *acl)
{
spin_lock(&inode->i_lock);
if (*i_acl != EXT3_ACL_NOT_CACHED)
@@ -167,23 +167,23 @@ ext3_get_acl(struct inode *inode, int type)
if (!test_opt(inode->i_sb, POSIX_ACL))
return NULL;

- switch(type) {
- case ACL_TYPE_ACCESS:
- acl = ext3_iget_acl(inode, &ei->i_acl);
- if (acl != EXT3_ACL_NOT_CACHED)
- return acl;
- name_index = EXT3_XATTR_INDEX_POSIX_ACL_ACCESS;
- break;
-
- case ACL_TYPE_DEFAULT:
- acl = ext3_iget_acl(inode, &ei->i_default_acl);
- if (acl != EXT3_ACL_NOT_CACHED)
- return acl;
- name_index = EXT3_XATTR_INDEX_POSIX_ACL_DEFAULT;
- break;
-
- default:
- return ERR_PTR(-EINVAL);
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ acl = ext3_iget_acl(inode, &ei->i_acl);
+ if (acl != EXT3_ACL_NOT_CACHED)
+ return acl;
+ name_index = EXT3_XATTR_INDEX_POSIX_ACL_ACCESS;
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ acl = ext3_iget_acl(inode, &ei->i_default_acl);
+ if (acl != EXT3_ACL_NOT_CACHED)
+ return acl;
+ name_index = EXT3_XATTR_INDEX_POSIX_ACL_DEFAULT;
+ break;
+
+ default:
+ return ERR_PTR(-EINVAL);
}
retval = ext3_xattr_get(inode, name_index, "", NULL, 0);
if (retval > 0) {
@@ -201,14 +201,14 @@ ext3_get_acl(struct inode *inode, int type)
kfree(value);

if (!IS_ERR(acl)) {
- switch(type) {
- case ACL_TYPE_ACCESS:
- ext3_iset_acl(inode, &ei->i_acl, acl);
- break;
-
- case ACL_TYPE_DEFAULT:
- ext3_iset_acl(inode, &ei->i_default_acl, acl);
- break;
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ ext3_iset_acl(inode, &ei->i_acl, acl);
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ ext3_iset_acl(inode, &ei->i_default_acl, acl);
+ break;
}
}
return acl;
@@ -232,31 +232,31 @@ ext3_set_acl(handle_t *handle, struct inode *inode, int type,
if (S_ISLNK(inode->i_mode))
return -EOPNOTSUPP;

- switch(type) {
- case ACL_TYPE_ACCESS:
- name_index = EXT3_XATTR_INDEX_POSIX_ACL_ACCESS;
- if (acl) {
- mode_t mode = inode->i_mode;
- error = posix_acl_equiv_mode(acl, &mode);
- if (error < 0)
- return error;
- else {
- inode->i_mode = mode;
- ext3_mark_inode_dirty(handle, inode);
- if (error == 0)
- acl = NULL;
- }
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ name_index = EXT3_XATTR_INDEX_POSIX_ACL_ACCESS;
+ if (acl) {
+ mode_t mode = inode->i_mode;
+ error = posix_acl_equiv_mode(acl, &mode);
+ if (error < 0)
+ return error;
+ else {
+ inode->i_mode = mode;
+ ext3_mark_inode_dirty(handle, inode);
+ if (error == 0)
+ acl = NULL;
}
- break;
+ }
+ break;

- case ACL_TYPE_DEFAULT:
- name_index = EXT3_XATTR_INDEX_POSIX_ACL_DEFAULT;
- if (!S_ISDIR(inode->i_mode))
- return acl ? -EACCES : 0;
- break;
+ case ACL_TYPE_DEFAULT:
+ name_index = EXT3_XATTR_INDEX_POSIX_ACL_DEFAULT;
+ if (!S_ISDIR(inode->i_mode))
+ return acl ? -EACCES : 0;
+ break;

- default:
- return -EINVAL;
+ default:
+ return -EINVAL;
}
if (acl) {
value = ext3_acl_to_disk(acl, &size);
@@ -269,14 +269,14 @@ ext3_set_acl(handle_t *handle, struct inode *inode, int type,

kfree(value);
if (!error) {
- switch(type) {
- case ACL_TYPE_ACCESS:
- ext3_iset_acl(inode, &ei->i_acl, acl);
- break;
-
- case ACL_TYPE_DEFAULT:
- ext3_iset_acl(inode, &ei->i_default_acl, acl);
- break;
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ ext3_iset_acl(inode, &ei->i_acl, acl);
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ ext3_iset_acl(inode, &ei->i_default_acl, acl);
+ break;
}
}
return error;
@@ -375,7 +375,7 @@ int
ext3_acl_chmod(struct inode *inode)
{
struct posix_acl *acl, *clone;
- int error;
+ int error;

if (S_ISLNK(inode->i_mode))
return -EOPNOTSUPP;
@@ -393,7 +393,7 @@ ext3_acl_chmod(struct inode *inode)
handle_t *handle;
int retries = 0;

- retry:
+retry:
handle = ext3_journal_start(inode,
EXT3_DATA_TRANS_BLOCKS(inode->i_sb));
if (IS_ERR(handle)) {
diff --git a/fs/ext3/acl.h b/fs/ext3/acl.h
index 0d1e627..ee85815 100644
--- a/fs/ext3/acl.h
+++ b/fs/ext3/acl.h
@@ -58,9 +58,9 @@ static inline int ext3_acl_count(size_t size)
#define EXT3_ACL_NOT_CACHED ((void *)-1)

/* acl.c */
-extern int ext3_permission (struct inode *, int, struct nameidata *);
-extern int ext3_acl_chmod (struct inode *);
-extern int ext3_init_acl (handle_t *, struct inode *, struct inode *);
+extern int ext3_permission(struct inode *, int, struct nameidata *);
+extern int ext3_acl_chmod(struct inode *);
+extern int ext3_init_acl(handle_t *, struct inode *, struct inode *);

#else /* CONFIG_EXT3_FS_POSIX_ACL */
#include <linux/sched.h>
diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c
index a8bae8c..f9f8d1e 100644
--- a/fs/ext4/acl.c
+++ b/fs/ext4/acl.c
@@ -40,34 +40,34 @@ ext4_acl_from_disk(const void *value, size_t size)
acl = posix_acl_alloc(count, GFP_KERNEL);
if (!acl)
return ERR_PTR(-ENOMEM);
- for (n=0; n < count; n++) {
+ for (n = 0; n < count; n++) {
ext4_acl_entry *entry =
(ext4_acl_entry *)value;
if ((char *)value + sizeof(ext4_acl_entry_short) > end)
goto fail;
acl->a_entries[n].e_tag = le16_to_cpu(entry->e_tag);
acl->a_entries[n].e_perm = le16_to_cpu(entry->e_perm);
- switch(acl->a_entries[n].e_tag) {
- case ACL_USER_OBJ:
- case ACL_GROUP_OBJ:
- case ACL_MASK:
- case ACL_OTHER:
- value = (char *)value +
- sizeof(ext4_acl_entry_short);
- acl->a_entries[n].e_id = ACL_UNDEFINED_ID;
- break;
-
- case ACL_USER:
- case ACL_GROUP:
- value = (char *)value + sizeof(ext4_acl_entry);
- if ((char *)value > end)
- goto fail;
- acl->a_entries[n].e_id =
- le32_to_cpu(entry->e_id);
- break;
-
- default:
+ switch (acl->a_entries[n].e_tag) {
+ case ACL_USER_OBJ:
+ case ACL_GROUP_OBJ:
+ case ACL_MASK:
+ case ACL_OTHER:
+ value = (char *)value +
+ sizeof(ext4_acl_entry_short);
+ acl->a_entries[n].e_id = ACL_UNDEFINED_ID;
+ break;
+
+ case ACL_USER:
+ case ACL_GROUP:
+ value = (char *)value + sizeof(ext4_acl_entry);
+ if ((char *)value > end)
goto fail;
+ acl->a_entries[n].e_id =
+ le32_to_cpu(entry->e_id);
+ break;
+
+ default:
+ goto fail;
}
}
if (value != end)
@@ -96,27 +96,27 @@ ext4_acl_to_disk(const struct posix_acl *acl, size_t *size)
return ERR_PTR(-ENOMEM);
ext_acl->a_version = cpu_to_le32(EXT4_ACL_VERSION);
e = (char *)ext_acl + sizeof(ext4_acl_header);
- for (n=0; n < acl->a_count; n++) {
+ for (n = 0; n < acl->a_count; n++) {
ext4_acl_entry *entry = (ext4_acl_entry *)e;
entry->e_tag = cpu_to_le16(acl->a_entries[n].e_tag);
entry->e_perm = cpu_to_le16(acl->a_entries[n].e_perm);
- switch(acl->a_entries[n].e_tag) {
- case ACL_USER:
- case ACL_GROUP:
- entry->e_id =
- cpu_to_le32(acl->a_entries[n].e_id);
- e += sizeof(ext4_acl_entry);
- break;
-
- case ACL_USER_OBJ:
- case ACL_GROUP_OBJ:
- case ACL_MASK:
- case ACL_OTHER:
- e += sizeof(ext4_acl_entry_short);
- break;
-
- default:
- goto fail;
+ switch (acl->a_entries[n].e_tag) {
+ case ACL_USER:
+ case ACL_GROUP:
+ entry->e_id =
+ cpu_to_le32(acl->a_entries[n].e_id);
+ e += sizeof(ext4_acl_entry);
+ break;
+
+ case ACL_USER_OBJ:
+ case ACL_GROUP_OBJ:
+ case ACL_MASK:
+ case ACL_OTHER:
+ e += sizeof(ext4_acl_entry_short);
+ break;
+
+ default:
+ goto fail;
}
}
return (char *)ext_acl;
@@ -167,23 +167,23 @@ ext4_get_acl(struct inode *inode, int type)
if (!test_opt(inode->i_sb, POSIX_ACL))
return NULL;

- switch(type) {
- case ACL_TYPE_ACCESS:
- acl = ext4_iget_acl(inode, &ei->i_acl);
- if (acl != EXT4_ACL_NOT_CACHED)
- return acl;
- name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
- break;
-
- case ACL_TYPE_DEFAULT:
- acl = ext4_iget_acl(inode, &ei->i_default_acl);
- if (acl != EXT4_ACL_NOT_CACHED)
- return acl;
- name_index = EXT4_XATTR_INDEX_POSIX_ACL_DEFAULT;
- break;
-
- default:
- return ERR_PTR(-EINVAL);
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ acl = ext4_iget_acl(inode, &ei->i_acl);
+ if (acl != EXT4_ACL_NOT_CACHED)
+ return acl;
+ name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ acl = ext4_iget_acl(inode, &ei->i_default_acl);
+ if (acl != EXT4_ACL_NOT_CACHED)
+ return acl;
+ name_index = EXT4_XATTR_INDEX_POSIX_ACL_DEFAULT;
+ break;
+
+ default:
+ return ERR_PTR(-EINVAL);
}
retval = ext4_xattr_get(inode, name_index, "", NULL, 0);
if (retval > 0) {
@@ -201,14 +201,14 @@ ext4_get_acl(struct inode *inode, int type)
kfree(value);

if (!IS_ERR(acl)) {
- switch(type) {
- case ACL_TYPE_ACCESS:
- ext4_iset_acl(inode, &ei->i_acl, acl);
- break;
-
- case ACL_TYPE_DEFAULT:
- ext4_iset_acl(inode, &ei->i_default_acl, acl);
- break;
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ ext4_iset_acl(inode, &ei->i_acl, acl);
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ ext4_iset_acl(inode, &ei->i_default_acl, acl);
+ break;
}
}
return acl;
@@ -232,31 +232,31 @@ ext4_set_acl(handle_t *handle, struct inode *inode, int type,
if (S_ISLNK(inode->i_mode))
return -EOPNOTSUPP;

- switch(type) {
- case ACL_TYPE_ACCESS:
- name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
- if (acl) {
- mode_t mode = inode->i_mode;
- error = posix_acl_equiv_mode(acl, &mode);
- if (error < 0)
- return error;
- else {
- inode->i_mode = mode;
- ext4_mark_inode_dirty(handle, inode);
- if (error == 0)
- acl = NULL;
- }
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
+ if (acl) {
+ mode_t mode = inode->i_mode;
+ error = posix_acl_equiv_mode(acl, &mode);
+ if (error < 0)
+ return error;
+ else {
+ inode->i_mode = mode;
+ ext4_mark_inode_dirty(handle, inode);
+ if (error == 0)
+ acl = NULL;
}
- break;
+ }
+ break;

- case ACL_TYPE_DEFAULT:
- name_index = EXT4_XATTR_INDEX_POSIX_ACL_DEFAULT;
- if (!S_ISDIR(inode->i_mode))
- return acl ? -EACCES : 0;
- break;
+ case ACL_TYPE_DEFAULT:
+ name_index = EXT4_XATTR_INDEX_POSIX_ACL_DEFAULT;
+ if (!S_ISDIR(inode->i_mode))
+ return acl ? -EACCES : 0;
+ break;

- default:
- return -EINVAL;
+ default:
+ return -EINVAL;
}
if (acl) {
value = ext4_acl_to_disk(acl, &size);
@@ -269,14 +269,14 @@ ext4_set_acl(handle_t *handle, struct inode *inode, int type,

kfree(value);
if (!error) {
- switch(type) {
- case ACL_TYPE_ACCESS:
- ext4_iset_acl(inode, &ei->i_acl, acl);
- break;
-
- case ACL_TYPE_DEFAULT:
- ext4_iset_acl(inode, &ei->i_default_acl, acl);
- break;
+ switch (type) {
+ case ACL_TYPE_ACCESS:
+ ext4_iset_acl(inode, &ei->i_acl, acl);
+ break;
+
+ case ACL_TYPE_DEFAULT:
+ ext4_iset_acl(inode, &ei->i_default_acl, acl);
+ break;
}
}
return error;
@@ -393,7 +393,7 @@ ext4_acl_chmod(struct inode *inode)
handle_t *handle;
int retries = 0;

- retry:
+retry:
handle = ext4_journal_start(inode,
EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
if (IS_ERR(handle)) {
diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
index 26a5c1a..7eac965 100644
--- a/fs/ext4/acl.h
+++ b/fs/ext4/acl.h
@@ -58,9 +58,9 @@ static inline int ext4_acl_count(size_t size)
#define EXT4_ACL_NOT_CACHED ((void *)-1)

/* acl.c */
-extern int ext4_permission (struct inode *, int, struct nameidata *);
-extern int ext4_acl_chmod (struct inode *);
-extern int ext4_init_acl (handle_t *, struct inode *, struct inode *);
+extern int ext4_permission(struct inode *, int, struct nameidata *);
+extern int ext4_acl_chmod(struct inode *);
+extern int ext4_init_acl(handle_t *, struct inode *, struct inode *);

#else /* CONFIG_EXT4DEV_FS_POSIX_ACL */
#include <linux/sched.h>
--
1.5.3.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tytso at mit

Jan 4, 2008, 5:44 AM

Post #2 of 13 (277 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

Coding-style only changes tends to screw up our ability to merge
pending patches, but I'll take care of it, thanks.

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mathieu.segaud at regala

Jan 4, 2008, 5:49 AM

Post #3 of 13 (278 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

Vous m'avez dit récemment :

> Coding-style only changes tends to screw up our ability to merge
> pending patches, but I'll take care of it, thanks

yep, I noticed that...
would you rather me wait till 2.6.24 is out ?

--
Mathieu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tytso at mit

Jan 4, 2008, 5:56 AM

Post #4 of 13 (277 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Fri, Jan 04, 2008 at 02:49:07PM +0100, Mathieu SEGAUD wrote:
> Vous m'avez dit récemment :
>
> > Coding-style only changes tends to screw up our ability to merge
> > pending patches, but I'll take care of it, thanks
>
> yep, I noticed that...
> would you rather me wait till 2.6.24 is out ?

I'll take your patches and merge all or most of it as I can into the
patch queue destined for 2.6.24-rc1. Check back with me after -rc1;
if there are some left over we'll do them after that.

- Ted

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andi at firstfloor

Jan 4, 2008, 8:30 AM

Post #5 of 13 (270 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

Theodore Tso <tytso[at]mit.edu> writes:

> Coding-style only changes tends to screw up our ability to merge
> pending patches, but I'll take care of it, thanks.

Exactly. And looking at the patch the old code was already perfectly
readable anyways. Benefit about zero.

I also don't see how you can take care of patch conflicts caused by
this for patches not yet in your tree but still in development somewhere
else.

IMHO any coding style cleanup should only done on code that changes
anyways, but not on other code.

The recent flurry of cleanup code patches on l-k causes far more
problems than it solves. I'm not even sure why people do this? Just
because it is en vogue recently?

If they want to contribute in simple ways to the Linux kernel I'm sure
actually really useful things can be found that they can change.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tytso at mit

Jan 4, 2008, 11:01 AM

Post #6 of 13 (270 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Fri, Jan 04, 2008 at 05:30:00PM +0100, Andi Kleen wrote:
>
> Exactly. And looking at the patch the old code was already perfectly
> readable anyways. Benefit about zero.

File this under the "checkpatch.pl" considered harmful category....
The problem is not with the tool, but that at least *some* people seem
to think that making checkpatch.pl be completely silent is somehow
this holy grail that will make kernel code bug-free(tm). (And of
course, people who want to encode nazi-like coding conventions and to
force all of the kernel to use a single coding convention as if that
somehow would improve the kernel's quality. Some of that is OK, but
as long as the code is readable, do we really care about whether or
not the code is using exactly the same coding conventions everyplace?
Or, pressuring maintainers not to ignore cleanup patches lest they be
viewed as "bad" maintainers?)

Personally I find it annoying, but I'm willing to live with the
cleanup patches. I don't think they add anything, though. Maybe I
should be more cranky about such patches....

> The recent flurry of cleanup code patches on l-k causes far more
> problems than it solves. I'm not even sure why people do this? Just
> because it is en vogue recently?

I don't know, because people want to be able to say that they've
contributed fixes to the Linux kernel?

I will say that in past kernel summit program commitees, the
perception that someone _only_ submitted trivial patches (i.e.,
whitespace-only, spelling fixes in comments, etc.) has sometimes been
perceived a negative factor towards whether the program committee
might consider that person to be useful contributor to discussions at
the kernel summit.... So people should be warned (I would have hoped
that it would be obvious), that submitting vast number of trivial
cleanup patches without contributing anything else will very likely
not work, and possibly backfire if that is your goal.

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andi at firstfloor

Jan 4, 2008, 11:41 AM

Post #7 of 13 (271 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

> Personally I find it annoying, but I'm willing to live with the
> cleanup patches. I don't think they add anything, though. Maybe I

The problem I see is that if someone has a more involved outstanding
patch series against the code that is being cleaned up (and more complicated
features tend to require some time to stabilize so "just merge early"
is not always the solution) then it is a serious mess to readapt
a patch series to the cleanups. Yes it can be all done but it wastes
time that would be more constructively used e.g. for better testing.

Now if some area is changed anyways then they're usually ok because
all outstanding patches will need to be adapted anyways.

So I guess a useful rule for cleanup patches would be "only if that
code changed recently"

> > The recent flurry of cleanup code patches on l-k causes far more
> > problems than it solves. I'm not even sure why people do this? Just
> > because it is en vogue recently?
>
> I don't know, because people want to be able to say that they've
> contributed fixes to the Linux kernel?

My pet theory is that it is similar to the "unsubscribe me"
cascade effect you sometimes see on mailing lists. One person
sends a "unsubscribe me" to everybody and then suddenly a lot of
people think that is the right way to unsubscribe and reply
with lots of "unsubscribe me too".

So one person sends a cleanup and it gets accepted and suddenly
other people realize it is very easy to do these cleanups
(not realizing the hidden costs they have) and then they go on...

I thought we had the janitor project to steer these people into
more useful directions, but apparently that is not well known
enough anymore. Perhaps it just needs to be more regularly announced?

Although I must admit I am not 100% happy with kernel-janitors
either -- e.g. a few times I sent suggestions about easy things
someone could do to that list, but never heard anything back.

Anyways there are lots of ways to do trivial cleanups in a useful
way and if people want to do this perhaps they should just
ask on linux-kernel and people suggest something?

My hope here is of course that these trivial changes are primarily
used as a way to get "the feet wet" to understand the procedures
for contribuing larger not quite as trivial changes

-Andi

P.S.: Mathieu, this is not against you personally; you just happened
to be a convenient example of a larger problem in this case. Sorry.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


gorcunov at gmail

Jan 4, 2008, 12:01 PM

Post #8 of 13 (270 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

[Andi Kleen - Fri, Jan 04, 2008 at 08:41:29PM +0100]
[...snip...]
| My hope here is of course that these trivial changes are primarily
| used as a way to get "the feet wet" to understand the procedures
| for contribuing larger not quite as trivial changes
|
| -Andi
|
| P.S.: Mathieu, this is not against you personally; you just happened
| to be a convenient example of a larger problem in this case. Sorry.
|

Oh, Andi, you're so right! I don't like to admit that, but you're
right (in my case at least). I counted four clean-up patches I sent
since last 'really bug fix' I made patch. Actually I thought that
really helps the kernel but it seems it doesn't and causes more
problem with patch applience (unfortunately).

- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paolo.ciarrocchi at gmail

Jan 4, 2008, 12:03 PM

Post #9 of 13 (270 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Jan 4, 2008 8:41 PM, Andi Kleen <andi[at]firstfloor.org> wrote:
[...]
> > I don't know, because people want to be able to say that they've
> > contributed fixes to the Linux kernel?
>
> My pet theory is that it is similar to the "unsubscribe me"
> cascade effect you sometimes see on mailing lists. One person
> sends a "unsubscribe me" to everybody and then suddenly a lot of
> people think that is the right way to unsubscribe and reply
> with lots of "unsubscribe me too".
>
> So one person sends a cleanup and it gets accepted and suddenly
> other people realize it is very easy to do these cleanups
> (not realizing the hidden costs they have) and then they go on...

Since I'm one of those people that sent "Codying style fixes" patches
I give my contribution to this discussion as well.

I think that _one_ of the reasons that made a few people sent this kind of
patches to the list is because checkpatch.pl is far better then any other
kerneljanitor scripts/easy task and _seems_ to be an easy way to start
understanding the code, creation of patches and process in general.

> I thought we had the janitor project to steer these people into
> more useful directions, but apparently that is not well known
> enough anymore. Perhaps it just needs to be more regularly announced?
>
> Although I must admit I am not 100% happy with kernel-janitors
> either -- e.g. a few times I sent suggestions about easy things
> someone could do to that list, but never heard anything back.
>
> Anyways there are lots of ways to do trivial cleanups in a useful
> way and if people want to do this perhaps they should just
> ask on linux-kernel and people suggest something?

Yes please do that.
Even if fixing errors reported by checkpatch.pl still sounds like a
useful way to spent a couple of hours.
Maybe our mistake was to send the patches to lkml instead of to
trivial[at]kernel.org
or to kerneljanitors?

I mean, I now understand the rationales behind your complaints but I
don't think it's
good idea to discourage people willing to perform easy task.
They just need guidance in order to be useful.

> My hope here is of course that these trivial changes are primarily
> used as a way to get "the feet wet" to understand the procedures
> for contribuing larger not quite as trivial changes

Agreed.

ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andi at firstfloor

Jan 4, 2008, 2:33 PM

Post #10 of 13 (270 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Fri, Jan 04, 2008 at 09:03:53PM +0100, Paolo Ciarrocchi wrote:
> On Jan 4, 2008 8:41 PM, Andi Kleen <andi[at]firstfloor.org> wrote:
> [...]
> > > I don't know, because people want to be able to say that they've
> > > contributed fixes to the Linux kernel?
> >
> > My pet theory is that it is similar to the "unsubscribe me"
> > cascade effect you sometimes see on mailing lists. One person
> > sends a "unsubscribe me" to everybody and then suddenly a lot of
> > people think that is the right way to unsubscribe and reply
> > with lots of "unsubscribe me too".
> >
> > So one person sends a cleanup and it gets accepted and suddenly
> > other people realize it is very easy to do these cleanups
> > (not realizing the hidden costs they have) and then they go on...
>
> Since I'm one of those people that sent "Codying style fixes" patches
> I give my contribution to this discussion as well.
>
> I think that _one_ of the reasons that made a few people sent this kind of
> patches to the list is because checkpatch.pl is far better then any other
> kerneljanitor scripts/easy task and _seems_ to be an easy way to start
> understanding the code, creation of patches and process in general.

The problem is that it has large hidden costs as pointed out. So while
it might be easy for you it's not a cheap operation for the whole development
process.

>
> > I thought we had the janitor project to steer these people into
> > more useful directions, but apparently that is not well known
> > enough anymore. Perhaps it just needs to be more regularly announced?
> >
> > Although I must admit I am not 100% happy with kernel-janitors
> > either -- e.g. a few times I sent suggestions about easy things
> > someone could do to that list, but never heard anything back.
> >
> > Anyways there are lots of ways to do trivial cleanups in a useful
> > way and if people want to do this perhaps they should just
> > ask on linux-kernel and people suggest something?
>
> Yes please do that.
> Even if fixing errors reported by checkpatch.pl still sounds like a
> useful way to spent a couple of hours.
> Maybe our mistake was to send the patches to lkml instead of to
> trivial[at]kernel.org
> or to kerneljanitors?

No, they would have ended in tree too eventually and caused the same problem.

How about if you're looking for simple work for a few hours you just
send an email to l-k and ask if someone has an idea for something?
I'm sure you'll get suggestions. Probably more than you can take.

e.g. from the top of my hat what would be useful:

- Go through Documentation/* files and check if the options etc. described
in there are still in the code

That will actually require you to find code in the source tree and understand
it at least a little bit which are both very useful skills in general.

- Or check for kerneldoc comments that do not appear in the kerneldoc output
(because the files are missing in the DocBook templates)

- Or build the kernel and check for any "deprecated" warnings and fix them
[.perhaps not 100% trivial, but should be doable by studying other code
a bit -- i expect that people who attempt to write such patches have at least
some knowledge of programming and C so that should be possible]

> I mean, I now understand the rationales behind your complaints but I
> don't think it's
> good idea to discourage people willing to perform easy task.
> They just need guidance in order to be useful.

Yes, the best way to get guidance is to ask.

-Andi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paolo.ciarrocchi at gmail

Jan 4, 2008, 4:12 PM

Post #11 of 13 (269 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Jan 4, 2008 11:33 PM, Andi Kleen <andi[at]firstfloor.org> wrote:
[...]
> > I think that _one_ of the reasons that made a few people sent this kind of
> > patches to the list is because checkpatch.pl is far better then any other
> > kerneljanitor scripts/easy task and _seems_ to be an easy way to start
> > understanding the code, creation of patches and process in general.
>
> The problem is that it has large hidden costs as pointed out. So while
> it might be easy for you it's not a cheap operation for the whole development
> process.

Isn't it a timing problem?
I mean, I guess that codying style fixes are OK if there is a good coordination
with the maintainer and patches are sent with the right timing in
order to not cause
problems in the process.
Do you agree?

May be, similar as you suggested, next time people should ask on the list
and fixing the codying style issues on the files suggested by the relevant
maintainers?

[...]
>
> How about if you're looking for simple work for a few hours you just
> send an email to l-k and ask if someone has an idea for something?
> I'm sure you'll get suggestions. Probably more than you can take.
>
> e.g. from the top of my hat what would be useful:
>
> - Go through Documentation/* files and check if the options etc. described
> in there are still in the code
>
> That will actually require you to find code in the source tree and understand
> it at least a little bit which are both very useful skills in general.
>
> - Or check for kerneldoc comments that do not appear in the kerneldoc output
> (because the files are missing in the DocBook templates)
>
> - Or build the kernel and check for any "deprecated" warnings and fix them
> [.perhaps not 100% trivial, but should be doable by studying other code
> a bit -- i expect that people who attempt to write such patches have at least
> some knowledge of programming and C so that should be possible]

OK, thanks for the hints!

> > I mean, I now understand the rationales behind your complaints but I
> > don't think it's
> > good idea to discourage people willing to perform easy task.
> > They just need guidance in order to be useful.
>
> Yes, the best way to get guidance is to ask.

Ciao,
--
Paolo
http://paolo.ciarrocchi.googlepages.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


tytso at mit

Jan 4, 2008, 4:39 PM

Post #12 of 13 (269 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Sat, Jan 05, 2008 at 01:12:44AM +0100, Paolo Ciarrocchi wrote:
> Isn't it a timing problem?
> I mean, I guess that codying style fixes are OK if there is a good coordination
> with the maintainer and patches are sent with the right timing in
> order to not cause
> problems in the process.

But because running some kind of mechanical script and fixing up the
problems is relatively mindless, it doesn't *add* anything. Only the
maintainer knows when it is a reasonably convenient time to fix all of
the problems, or when to fix portions of the code that is being
reworked anyway --- and the maintainer can just run the script by
himself, for himself. The patch doesn't actually save him much work,
and in some cases, is actually more work than simply regenerating the
fixes *after* the other patches waiting in his patch queue have been
applied (but of course, it screws up everyone else's patches that
haven't been submitted to the maintainer yet).

In some cases, it's worth it. In other (most) cases, it really isn't.

- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


jengelh at computergmbh

Jan 5, 2008, 1:24 PM

Post #13 of 13 (266 views)
Permalink
Re: [PATCH] [Coding Style]: misc fixes for fs/ext{3,4}/acl.{c,h} from checkpatch.pl [In reply to]

On Jan 4 2008 19:39, Theodore Tso wrote:
>On Sat, Jan 05, 2008 at 01:12:44AM +0100, Paolo Ciarrocchi wrote:
>
>But because running some kind of mechanical script and fixing up the
>problems is relatively mindless, it doesn't *add* anything. Only the
>maintainer knows when it is a reasonably convenient time to fix all of
>the problems, or when to fix portions of the code that is being
>reworked anyway --- and the maintainer can just run the script by
>himself, for himself.

I have to agree. Best use of checkpatch is right before submitting, uh,
a patch actually. The option that makes it work on non-patch files
should be, hm, hidden or removed.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo[at]vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact lists@gossamer-threads.com
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.