LinuxLists.cc - [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

[permalink] [raw]

Subject: [PATCH 04/10] Fs: ext4: block_validity: added space around = sign

From: Tracey Dent <[email protected]>

Added space around = sign. Checkpatch.pl detected the issue

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/block_validity.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 3db5084..25e9f52 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -156,7 +156,7 @@ int ext4_setup_system_zone(struct super_block *sb)
if (EXT4_SB(sb)->system_blks.rb_node)
return 0;

- for (i=0; i < ngroups; i++) {
+ for (i = 0; i < ngroups; i++) {
if (ext4_bg_has_super(sb, i) &&
((i < 5) || ((i % flex_size) == 0)))
add_system_zone(sbi, ext4_group_first_block_no(sb, i),
--
1.7.3

2010-09-25 18:32:53

[permalink] [raw]

Subject: [PATCH 02/10] Fs: ext4: acl.h: whitespace cleanup

From: Tracey Dent <[email protected]>

I cleaned up whitespaces that was having an issue with checkpatch.pl

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/acl.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/acl.h b/fs/ext4/acl.h
index 9d843d5..bb225cf 100644
--- a/fs/ext4/acl.h
+++ b/fs/ext4/acl.h
@@ -1,7 +1,7 @@
/*
- File: fs/ext4/acl.h
+ File: fs/ext4/acl.h

- (C) 2001 Andreas Gruenbacher, <[email protected]>
+ (C) 2001 Andreas Gruenbacher, <[email protected]>
*/

#include <linux/posix_acl_xattr.h>
--
1.7.3

2010-09-25 18:31:59

[permalink] [raw]

Subject: [PATCH 08/10] Fs: ext4: ioctl: fixed spacing issue

From: Tracey Dent <[email protected]>

Fixed spaceing issues using checkpatch.pl

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/ioctl.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index bf5ae88..4e3ed22 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -200,7 +200,7 @@ setversion_out:
case EXT4_IOC_GROUP_EXTEND: {
ext4_fsblk_t n_blocks_count;
struct super_block *sb = inode->i_sb;
- int err, err2=0;
+ int err, err2 = 0;

if (!capable(CAP_SYS_RESOURCE))
return -EPERM;
@@ -269,7 +269,7 @@ mext_out:
case EXT4_IOC_GROUP_ADD: {
struct ext4_new_group_data input;
struct super_block *sb = inode->i_sb;
- int err, err2=0;
+ int err, err2 = 0;

if (!capable(CAP_SYS_RESOURCE))
return -EPERM;
--
1.7.3

2010-09-25 18:31:56

[permalink] [raw]

Subject: [PATCH 05/10] Fs: ext4: ext4: cleaned up the file with checkpatch.pl

From: Tracey Dent <[email protected]>

Fixed numerous issues that checkpatch.pl was having with the file

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/ext4.h | 43 ++++++++++++++++++++-----------------------
1 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 889ec9d..6aa5871 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -233,8 +233,7 @@ typedef struct ext4_io_end {
/*
* Structure of a blocks group descriptor
*/
-struct ext4_group_desc
-{
+struct ext4_group_desc {
__le32 bg_block_bitmap_lo; /* Blocks bitmap block */
__le32 bg_inode_bitmap_lo; /* Inodes bitmap block */
__le32 bg_inode_table_lo; /* Inodes table block */
@@ -513,8 +512,8 @@ struct ext4_new_group_data {
#define EXT4_IOC_GROUP_EXTEND _IOW('f', 7, unsigned long)
#define EXT4_IOC_GROUP_ADD _IOW('f', 8, struct ext4_new_group_input)
#define EXT4_IOC_MIGRATE _IO('f', 9)
- /* note ioctl 10 reserved for an early version of the FIEMAP ioctl */
- /* note ioctl 11 reserved for filesystem-independent FIEMAP ioctl */
+ /* note ioctl 10 reserved for an early version of the FIEMAP ioctl */
+ /* note ioctl 11 reserved for filesystem-independent FIEMAP ioctl */
#define EXT4_IOC_ALLOC_DA_BLKS _IO('f', 12)
#define EXT4_IOC_MOVE_EXT _IOWR('f', 15, struct move_extent)

@@ -649,17 +648,17 @@ struct move_extent {

static inline __le32 ext4_encode_extra_time(struct timespec *time)
{
- return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
- (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
- ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));
+ return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
+ (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
+ ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));
}

static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra)
{
- if (sizeof(time->tv_sec) > 4)
+ if (sizeof(time->tv_sec) > 4)
time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK)
<< 32;
- time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
+ time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS;
}

#define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \
@@ -1261,23 +1260,23 @@ EXT4_INODE_BIT_FNS(state, state_flags)
* Feature set definitions
*/

-#define EXT4_HAS_COMPAT_FEATURE(sb,mask) \
+#define EXT4_HAS_COMPAT_FEATURE(sb, mask) \
((EXT4_SB(sb)->s_es->s_feature_compat & cpu_to_le32(mask)) != 0)
-#define EXT4_HAS_RO_COMPAT_FEATURE(sb,mask) \
+#define EXT4_HAS_RO_COMPAT_FEATURE(sb, mask) \
((EXT4_SB(sb)->s_es->s_feature_ro_compat & cpu_to_le32(mask)) != 0)
-#define EXT4_HAS_INCOMPAT_FEATURE(sb,mask) \
+#define EXT4_HAS_INCOMPAT_FEATURE(sb, mask) \
((EXT4_SB(sb)->s_es->s_feature_incompat & cpu_to_le32(mask)) != 0)
-#define EXT4_SET_COMPAT_FEATURE(sb,mask) \
+#define EXT4_SET_COMPAT_FEATURE(sb, mask) \
EXT4_SB(sb)->s_es->s_feature_compat |= cpu_to_le32(mask)
-#define EXT4_SET_RO_COMPAT_FEATURE(sb,mask) \
+#define EXT4_SET_RO_COMPAT_FEATURE(sb, mask) \
EXT4_SB(sb)->s_es->s_feature_ro_compat |= cpu_to_le32(mask)
-#define EXT4_SET_INCOMPAT_FEATURE(sb,mask) \
+#define EXT4_SET_INCOMPAT_FEATURE(sb, mask) \
EXT4_SB(sb)->s_es->s_feature_incompat |= cpu_to_le32(mask)
-#define EXT4_CLEAR_COMPAT_FEATURE(sb,mask) \
+#define EXT4_CLEAR_COMPAT_FEATURE(sb, mask) \
EXT4_SB(sb)->s_es->s_feature_compat &= ~cpu_to_le32(mask)
-#define EXT4_CLEAR_RO_COMPAT_FEATURE(sb,mask) \
+#define EXT4_CLEAR_RO_COMPAT_FEATURE(sb, mask) \
EXT4_SB(sb)->s_es->s_feature_ro_compat &= ~cpu_to_le32(mask)
-#define EXT4_CLEAR_INCOMPAT_FEATURE(sb,mask) \
+#define EXT4_CLEAR_INCOMPAT_FEATURE(sb, mask) \
EXT4_SB(sb)->s_es->s_feature_incompat &= ~cpu_to_le32(mask)

#define EXT4_FEATURE_COMPAT_DIR_PREALLOC 0x0001
@@ -1471,8 +1470,7 @@ static inline __le16 ext4_rec_len_to_disk(unsigned len, unsigned blocksize)
#ifdef __KERNEL__

/* hash info structure used by the directory hash */
-struct dx_hash_info
-{
+struct dx_hash_info {
u32 hash;
u32 minor_hash;
int hash_version;
@@ -1490,8 +1488,7 @@ struct dx_hash_info
/*
* Describe an inode's exact location on disk and in memory
*/
-struct ext4_iloc
-{
+struct ext4_iloc {
struct buffer_head *bh;
unsigned long offset;
ext4_group_t block_group;
@@ -1947,7 +1944,7 @@ static inline void ext4_unlock_group(struct super_block *sb,
static inline void ext4_mark_super_dirty(struct super_block *sb)
{
if (EXT4_SB(sb)->s_journal == NULL)
- sb->s_dirt =1;
+ sb->s_dirt = 1;
}

/*
--
1.7.3

2010-09-25 18:33:00

[permalink] [raw]

Subject: [PATCH 07/10] Fs: ext4: file: fixed indent problem

From: Tracey Dent <[email protected]>

Fixed indent problem that was detected by checkpatch.pl

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/file.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index ee92b66..d75e8e6 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -43,7 +43,7 @@ static int ext4_release_file(struct inode *inode, struct file *filp)
/* if we are the last writer on the inode, drop the block reservation */
if ((filp->f_mode & FMODE_WRITE) &&
(atomic_read(&inode->i_writecount) == 1) &&
- !EXT4_I(inode)->i_reserved_data_blocks)
+ !EXT4_I(inode)->i_reserved_data_blocks)
{
down_write(&EXT4_I(inode)->i_data_sem);
ext4_discard_preallocations(inode);
--
1.7.3

2010-09-25 18:32:00

[permalink] [raw]

Subject: [PATCH 09/10] Fs: ext4: mballoc.c: whitespace cleanup

From: Tracey Dent <[email protected]>

Fixed whitespace and indent issues that checkpatch.pl had an issue with

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/mballoc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 4b4ad4b..0bc545c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2410,9 +2410,9 @@ int ext4_mb_init(struct super_block *sb, int needs_recovery)
i = (sb->s_blocksize_bits + 2) * sizeof(*sbi->s_mb_offsets);

sbi->s_mb_offsets = kmalloc(i, GFP_KERNEL);
- if (sbi->s_mb_offsets == NULL) {
+ if (sbi->s_mb_offsets == NULL)
return -ENOMEM;
- }
+

i = (sb->s_blocksize_bits + 2) * sizeof(*sbi->s_mb_maxs);
sbi->s_mb_maxs = kmalloc(i, GFP_KERNEL);
@@ -3908,7 +3908,7 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
(int)ac->ac_criteria);
printk(KERN_ERR "EXT4-fs: %lu scanned, %d found\n", ac->ac_ex_scanned,
ac->ac_found);
- printk(KERN_ERR "EXT4-fs: groups: \n");
+ printk(KERN_ERR "EXT4-fs: groups:\n");
ngroups = ext4_get_groups_count(sb);
for (i = 0; i < ngroups; i++) {
struct ext4_group_info *grp = ext4_get_group_info(sb, i);
@@ -3923,14 +3923,14 @@ static void ext4_mb_show_ac(struct ext4_allocation_context *ac)
ext4_get_group_no_and_offset(sb, pa->pa_pstart,
NULL, &start);
spin_unlock(&pa->pa_lock);
- printk(KERN_ERR "PA:%u:%d:%u \n", i,
+ printk(KERN_ERR "PA:%u:%d:%u\n", i,
start, pa->pa_len);
}
ext4_unlock_group(sb, i);

if (grp->bb_free == 0)
continue;
- printk(KERN_ERR "%u: %d/%d \n",
+ printk(KERN_ERR "%u: %d/%d\n",
i, grp->bb_free, grp->bb_fragments);
}
printk(KERN_ERR "\n");
--
1.7.3

2010-09-25 18:33:05

[permalink] [raw]

Subject: [PATCH 10/10] Fs: ext4: namei: fixed file of checkpatch/pl warnings and errors

From: Tracey Dent <[email protected]>

Fixed numerous checkpat.pl errors and warnings

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/namei.c | 72 ++++++++++++++++++++----------------------------------
1 files changed, 27 insertions(+), 45 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 314c0d3..0efa236 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -46,7 +46,7 @@
#define NAMEI_RA_CHUNKS 2
#define NAMEI_RA_BLOCKS 4
#define NAMEI_RA_SIZE (NAMEI_RA_CHUNKS * NAMEI_RA_BLOCKS)
-#define NAMEI_RA_INDEX(c,b) (((c) * NAMEI_RA_BLOCKS) + (b))
+#define NAMEI_RA_INDEX(c, b) (((c) * NAMEI_RA_BLOCKS) + (b))

static struct buffer_head *ext4_append(handle_t *handle,
struct inode *inode,
@@ -79,22 +79,19 @@ static struct buffer_head *ext4_append(handle_t *handle,
#define dxtrace(command)
#endif

-struct fake_dirent
-{
+struct fake_dirent {
__le32 inode;
__le16 rec_len;
u8 name_len;
u8 file_type;
};

-struct dx_countlimit
-{
+struct dx_countlimit {
__le16 limit;
__le16 count;
};

-struct dx_entry
-{
+struct dx_entry {
__le32 hash;
__le32 block;
};
@@ -105,14 +102,12 @@ struct dx_entry
* hash version mod 4 should never be 0. Sincerely, the paranoia department.
*/

-struct dx_root
-{
+struct dx_root {
struct fake_dirent dot;
char dot_name[4];
struct fake_dirent dotdot;
char dotdot_name[4];
- struct dx_root_info
- {
+ struct dx_root_info {
__le32 reserved_zero;
u8 hash_version;
u8 info_length; /* 8 */
@@ -123,22 +118,19 @@ struct dx_root
struct dx_entry entries[0];
};

-struct dx_node
-{
+struct dx_node {
struct fake_dirent fake;
struct dx_entry entries[0];
};

-struct dx_frame
-{
+struct dx_frame {
struct buffer_head *bh;
struct dx_entry *entries;
struct dx_entry *at;
};

-struct dx_map_entry
-{
+struct dx_map_entry {
u32 hash;
u16 offs;
u16 size;
@@ -253,7 +245,7 @@ static inline unsigned dx_node_limit(struct inode *dir)
#ifdef DX_DEBUG
static void dx_show_index(char * label, struct dx_entry *entries)
{
- int i, n = dx_get_count (entries);
+ int i, n = dx_get_count(entries);
printk(KERN_DEBUG "%s index ", label);
for (i = 0; i < n; i++) {
printk("%x->%lu ", i ? dx_get_hash(entries + i) :
@@ -262,8 +254,7 @@ static void dx_show_index(char * label, struct dx_entry *entries)
printk("\n");
}

-struct stats
-{
+struct stats {
unsigned names;
unsigned space;
unsigned bcount;
@@ -277,12 +268,9 @@ static struct stats dx_show_leaf(struct dx_hash_info *hinfo, struct ext4_dir_ent
struct dx_hash_info h = *hinfo;

printk("names: ");
- while ((char *) de < base + size)
- {
- if (de->inode)
- {
- if (show_names)
- {
+ while ((char *) de < base + size) {
+ if (de->inode) {
+ if (show_names) {
int len = de->name_len;
char *name = de->name;
while (len--) printk("%c", *name++);
@@ -308,16 +296,15 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir,
struct buffer_head *bh;
int err;
printk("%i indexed blocks...\n", count);
- for (i = 0; i < count; i++, entries++)
- {
+ for (i = 0; i < count; i++, entries++) {
ext4_lblk_t block = dx_get_block(entries);
- ext4_lblk_t hash = i ? dx_get_hash(entries): 0;
- u32 range = i < count - 1? (dx_get_hash(entries + 1) - hash): ~hash;
+ ext4_lblk_t hash = i ? dx_get_hash(entries) : 0;
+ u32 range = i < count - 1 ? (dx_get_hash(entries + 1) - hash) : ~hash;
struct stats stats;
- printk("%s%3u:%03u hash %8x/%8x ",levels?"":" ", i, block, hash, range);
- if (!(bh = ext4_bread (NULL,dir, block, 0,&err))) continue;
- stats = levels?
- dx_show_entries(hinfo, dir, ((struct dx_node *) bh->b_data)->entries, levels - 1):
+ printk("%s%3u:%03u hash %8x/%8x ", levels ? "":" ", i, block, hash, range);
+ if (!(bh = ext4_bread(NULL,dir, block, 0,&err))) continue;
+ stats = levels ?
+ dx_show_entries(hinfo, dir, ((struct dx_node *) bh->b_data)->entries, levels - 1) :
dx_show_leaf(hinfo, (struct ext4_dir_entry_2 *) bh->b_data, blocksize, 0);
names += stats.names;
space += stats.space;
@@ -353,7 +340,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
u32 hash;

frame->bh = NULL;
- if (!(bh = ext4_bread (NULL,dir, 0, 0, err)))
+ if (!(bh = ext4_bread(NULL, dir, 0, 0, err)))
goto fail;
root = (struct dx_root *) bh->b_data;
if (root->info.hash_version != DX_HASH_TEA &&
@@ -401,8 +388,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
}

dxtrace(printk("Look up %x", hash));
- while (1)
- {
+ while (1) {
count = dx_get_count(entries);
if (!count || count > dx_get_limit(entries)) {
ext4_warning(dir->i_sb,
@@ -414,8 +400,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir,

p = entries + 1;
q = entries + count - 1;
- while (p <= q)
- {
+ while (p <= q) {
m = p + (q - p)/2;
dxtrace(printk("."));
if (dx_get_hash(m) > hash)
@@ -424,15 +409,12 @@ dx_probe(const struct qstr *d_name, struct inode *dir,
p = m + 1;
}

- if (0) // linear search cross check
- {
+ if (0) /* linear search cross check */ {
unsigned n = count - 1;
at = entries;
- while (n--)
- {
+ while (n--) {
dxtrace(printk(","));
- if (dx_get_hash(++at) > hash)
- {
+ if (dx_get_hash(++at) > hash) {
at--;
break;
}
--
1.7.3

2010-09-25 18:32:59

[permalink] [raw]

Subject: [PATCH 06/10] Fs: ext4: extents: whitespace cleanup

From: Tracey Dent <[email protected]>

Fixed whitespaces issues that checkpath.pl founded

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/extents.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 06328d3..06f7bb3 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -461,7 +461,7 @@ static void ext4_ext_show_path(struct inode *inode, struct ext4_ext_path *path)

ext_debug("path:");
for (k = 0; k <= l; k++, path++) {
- if (path->p_idx) {
+ if (path->p_idx) {
ext_debug(" %d->%llu", le32_to_cpu(path->p_idx->ei_block),
idx_pblock(path->p_idx));
} else if (path->p_ext) {
@@ -553,7 +553,7 @@ ext4_ext_binsearch_idx(struct inode *inode,
int k;

chix = ix = EXT_FIRST_INDEX(eh);
- for (k = 0; k < le16_to_cpu(eh->eh_entries); k++, ix++) {
+ for (k = 0; k < le16_to_cpu(eh->eh_entries); k++, ix++) {
if (k != 0 &&
le32_to_cpu(ix->ei_block) <= le32_to_cpu(ix[-1].ei_block)) {
printk(KERN_DEBUG "k=%d, ix=0x%p, "
@@ -1784,7 +1784,7 @@ has_space:
ext4_ext_is_uninitialized(newext),
ext4_ext_get_actual_len(newext));
path[depth].p_ext = EXT_FIRST_EXTENT(eh);
- } else if (le32_to_cpu(newext->ee_block)
+ } else if (le32_to_cpu(newext->ee_block)
> le32_to_cpu(nearex->ee_block)) {
/* BUG_ON(newext->ee_block == nearex->ee_block); */
if (nearex != EXT_LAST_EXTENT(eh)) {
@@ -3477,7 +3477,7 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
ext4_ext_store_pblock(&newex, newblock);
newex.ee_len = cpu_to_le16(ar.len);
/* Mark uninitialized */
- if (flags & EXT4_GET_BLOCKS_UNINIT_EXT){
+ if (flags & EXT4_GET_BLOCKS_UNINIT_EXT) {
ext4_ext_mark_uninitialized(&newex);
/*
* io_end structure was created for every IO write to an
@@ -3807,7 +3807,7 @@ int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
}
ext4_mark_inode_dirty(handle, inode);
ret2 = ext4_journal_stop(handle);
- if (ret <= 0 || ret2 )
+ if (ret <= 0 || ret2)
break;
}
return ret > 0 ? ret2 : ret;
--
1.7.3

2010-09-25 18:32:57

[permalink] [raw]

Subject: [PATCH 03/10] Fs: ext: balloc: fixed a few issues that checkpatch.pl was having

From: Tracey Dent <[email protected]>

Fixed space after a variable with help of checkpatch.pl

Signed-off-by: Tracey Dent <[email protected]>
---
fs/ext4/balloc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index bd30799..ca3a161 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -390,9 +390,9 @@ void ext4_add_groupblocks(handle_t *handle, struct super_block *sb,
* Check to see if we are freeing blocks across a group
* boundary.
*/
- if (bit + count > EXT4_BLOCKS_PER_GROUP(sb)) {
+ if (bit + count > EXT4_BLOCKS_PER_GROUP(sb))
goto error_return;
- }
+
bitmap_bh = ext4_read_block_bitmap(sb, block_group);
if (!bitmap_bh)
goto error_return;
@@ -710,7 +710,7 @@ static unsigned long ext4_bg_num_gdb_nometa(struct super_block *sb,
if (!ext4_bg_has_super(sb, group))
return 0;

- if (EXT4_HAS_INCOMPAT_FEATURE(sb,EXT4_FEATURE_INCOMPAT_META_BG))
+ if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_META_BG))
return le32_to_cpu(EXT4_SB(sb)->s_es->s_first_meta_bg);
else
return EXT4_SB(sb)->s_gdb_count;
@@ -731,11 +731,11 @@ unsigned long ext4_bg_num_gdb(struct super_block *sb, ext4_group_t group)
le32_to_cpu(EXT4_SB(sb)->s_es->s_first_meta_bg);
unsigned long metagroup = group / EXT4_DESC_PER_BLOCK(sb);

- if (!EXT4_HAS_INCOMPAT_FEATURE(sb,EXT4_FEATURE_INCOMPAT_META_BG) ||
+ if (!EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_META_BG) ||
metagroup < first_meta_bg)
return ext4_bg_num_gdb_nometa(sb, group);

- return ext4_bg_num_gdb_meta(sb,group);
+ return ext4_bg_num_gdb_meta(sb, group);

}

--
1.7.3

2010-09-25 23:37:15

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

Please don't Cc me on ext4 patches, thanks.

2010-09-25 23:53:04

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On 9/25/10, Christoph Hellwig <[email protected]> wrote:
> Please don't Cc me on ext4 patches, thanks.
>
>

Ok.. i just used the get maintainer script and your name was on it.

Sorry,
Tracey

2010-09-25 23:55:18

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 02:31:52PM -0400, Tracey Dent wrote:
> From: Tracey Dent <[email protected]>
>
> Found and corrected indent issue using checkpatch.pl
>
> Signed-off-by: Tracey Dent <[email protected]>

Patches that fix whitespace issues aren't really worthwhile. They
tend to cause extra work for the me as the maintainer, since it means
that patches that others send me end up failing due to whitespace
issues, which then have to be manually fixed up.

There are also changes you've made which actually make the alignment
*worse* not better, places where you changed:

} else if

to
} else if

which is to my mind, totally broken, and certainly not required by
checkpatch.

So I'm going to NACK this whole patch series, sorry.

Patches should be checkpatch.pl clean, and so as we make changes,
we'll also gradually clean up the code. Note though that I'm an emacs
user, and so I prefer code where parenthesis is properly lined up,
i.e.:

while ((count < blks) && (count <= blocks_to_boundary) &&
(le32_to_cpu(*(branch[0].p + count)) == 0)) {

and not this:

while ((count < blks) && (count <= blocks_to_boundary) &&
(le32_to_cpu(*(branch[0].p + count)) == 0)) {

I won't insist on it, though. However, I'll generally clean up such
alignment issues if I come across it.

; - Ted

2010-09-25 23:57:04

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 07:53:02PM -0400, T Dent wrote:
> On 9/25/10, Christoph Hellwig <[email protected]> wrote:
> > Please don't Cc me on ext4 patches, thanks.
> >
> >
>
> Ok.. i just used the get maintainer script and your name was on it.
>

The script is broken but the maintainer refuses to fix it.

2010-09-26 00:01:58

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sun, Sep 26, 2010 at 01:56:43AM +0200, Christoph Hellwig wrote:
> On Sat, Sep 25, 2010 at 07:53:02PM -0400, T Dent wrote:
> > On 9/25/10, Christoph Hellwig <[email protected]> wrote:
> > > Please don't Cc me on ext4 patches, thanks.
> > >
> > Ok.. i just used the get maintainer script and your name was on it.
> >
>
> The script is broken but the maintainer refuses to fix it.

Wow, I just looked at how get_maintainer.pl works, and it's amazing
how broken it is. I guess I had heard some rumblings of unhappiness
about it, but I'm surprised it's been allowed to live.

- Ted

2010-09-26 00:06:22

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sun, 2010-09-26 at 01:56 +0200, Christoph Hellwig wrote:
> On Sat, Sep 25, 2010 at 07:53:02PM -0400, T Dent wrote:
> > On 9/25/10, Christoph Hellwig <[email protected]> wrote:
> > > Please don't Cc me on ext4 patches, thanks.
> > Ok.. i just used the get maintainer script and your name was on it.
> The script is broken but the maintainer refuses to fix it.

That'd be false.

https://patchwork.kernel.org/patch/185352/

That version is in Andrew Morton's mm.

$ ./scripts/get_maintainer.pl -f fs/ext4/acl.c
"Theodore Ts'o" <[email protected]>
Andreas Dilger <[email protected]>
[email protected]

cheers, Joe

2010-09-26 00:09:32

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 20:01 -0400, Ted Ts'o wrote:
> On Sun, Sep 26, 2010 at 01:56:43AM +0200, Christoph Hellwig wrote:
> > On Sat, Sep 25, 2010 at 07:53:02PM -0400, T Dent wrote:
> > > On 9/25/10, Christoph Hellwig <[email protected]> wrote:
> > > > Please don't Cc me on ext4 patches, thanks.
> > > Ok.. i just used the get maintainer script and your name was on it.
> > The script is broken but the maintainer refuses to fix it.
> Wow, I just looked at how get_maintainer.pl works, and it's amazing
> how broken it is. I guess I had heard some rumblings of unhappiness
> about it, but I'm surprised it's been allowed to live.

In your opinion Ted, what's broken about it?

2010-09-26 00:32:55

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 05:09:29PM -0700, Joe Perches wrote:
> > Wow, I just looked at how get_maintainer.pl works, and it's amazing
> > how broken it is. I guess I had heard some rumblings of unhappiness
> > about it, but I'm surprised it's been allowed to live.
>
> In your opinion Ted, what's broken about it?
>

It's casting **way** too wide of a net:

./scripts/get_maintainer.pl --roles -f fs/ext4/inode.c
"Theodore Ts'o" <[email protected]> (maintainer:EXT4 FILE SYSTEM,commit_signer)
Andreas Dilger <[email protected]> (maintainer:EXT4 FILE SYSTEM)
Jan Kara <[email protected]> (commit_signer)
Eric Sandeen <[email protected]> (commit_signer)
Dmitry Monakhov <[email protected]> (commit_signer)
Christoph Hellwig <[email protected]> (commit_signer)
[email protected] (open list:EXT4 FILE SYSTEM)
[email protected] (open list)

The right answer is to just send the e-mail to the linux-ext4 list.
That's what the MAINTAINERS file says, and it's right. Using people
who have signed off of on commits, and blindly assuming that they are
therefore are maintainers, is Just Wrong.

Christoph has said he doesn't want to get e-mails about ext4 patches
--- and he shouldn't get them just because the get_maintainers.pl
script is broken.

- Ted

2010-09-26 00:36:12

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 08:32:55PM -0400, Ted Ts'o wrote:
> On Sat, Sep 25, 2010 at 05:09:29PM -0700, Joe Perches wrote:
> > > Wow, I just looked at how get_maintainer.pl works, and it's amazing
> > > how broken it is. I guess I had heard some rumblings of unhappiness
> > > about it, but I'm surprised it's been allowed to live.
> >
> > In your opinion Ted, what's broken about it?
> >
>
> It's casting **way** too wide of a net:

Oh, and the output on fs/ext4/acl.c must have been created by a
crack-addled monkey:

% ./scripts/get_maintainer.pl --roles -f fs/ext4/acl.c
"Theodore Ts'o" <[email protected]> (maintainer:EXT4 FILE SYSTEM)
Andreas Dilger <[email protected]> (maintainer:EXT4 FILE SYSTEM)
Al Viro <[email protected]> (commit_signer)
Stephen Hemminger <[email protected]> (commit_signer)
Joel Becker <[email protected]> (commit_signer)
James Morris <[email protected]> (commit_signer)
Jan Kara <[email protected]> (commit_signer)
[email protected] (open list:EXT4 FILE SYSTEM)
[email protected] (open list)

I'm pretty sure Stephen Hemminger, Joel Becker, and James Morris don't
want to get spammed by random ext4 e-mails.

- Ted

2010-09-26 00:50:43

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 20:32 -0400, Ted Ts'o wrote:
> On Sat, Sep 25, 2010 at 05:09:29PM -0700, Joe Perches wrote:
> > > Wow, I just looked at how get_maintainer.pl works, and it's amazing
> > > how broken it is. I guess I had heard some rumblings of unhappiness
> > > about it, but I'm surprised it's been allowed to live.
> > In your opinion Ted, what's broken about it?
> It's casting **way** too wide of a net:
> ./scripts/get_maintainer.pl --roles -f fs/ext4/inode.c
> "Theodore Ts'o" <[email protected]> (maintainer:EXT4 FILE SYSTEM,commit_signer)
> Andreas Dilger <[email protected]> (maintainer:EXT4 FILE SYSTEM)
> Jan Kara <[email protected]> (commit_signer)
> Eric Sandeen <[email protected]> (commit_signer)
> Dmitry Monakhov <[email protected]> (commit_signer)
> Christoph Hellwig <[email protected]> (commit_signer)
> [email protected] (open list:EXT4 FILE SYSTEM)
> [email protected] (open list)
>
> The right answer is to just send the e-mail to the linux-ext4 list.
> That's what the MAINTAINERS file says, and it's right.

What the MAINTAINERS file says is:

5. Make a patch available to the relevant maintainer in the list

It doesn't say to send emails only to any appropriate list.

The EXT4 section is:

EXT4 FILE SYSTEM
M: "Theodore Ts'o" <[email protected]>
M: Andreas Dilger <[email protected]>
L: [email protected]
W: http://ext4.wiki.kernel.org
Q: http://patchwork.ozlabs.org/project/linux-ext4/list/
S: Maintained
F: Documentation/filesystems/ext4.txt
F: fs/ext4/

> Using people
> who have signed off of on commits, and blindly assuming that they are
> therefore are maintainers, is Just Wrong.
>
> Christoph has said he doesn't want to get e-mails about ext4 patches
> --- and he shouldn't get them just because the get_maintainers.pl
> script is broken.

The reason get_maintainers by default cc'd signers is mostly
historical. The file pattern coverage in MAINTAINERS when
it was added wasn't very good, so signers were always added.
It was also the Linus' preferred mechanism to find those
"who really do the work".

http://lkml.org/lkml/2007/8/14/276

Anyway, the current file pattern coverage is probably
sufficiently good to change the --git default to off, as was
done by this commit already in Andrew Morton's mm tree.

Now git history is used only if there is no specifically
named maintainer.

The current version returns:

$ ./scripts/get_maintainer.pl -f fs/ext4/acl.c
"Theodore Ts'o" <[email protected]>
Andreas Dilger <[email protected]>
[email protected]
[email protected]

cheers, Joe

2010-09-26 00:58:44

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 17:50 -0700, Joe Perches wrote:
> Now git history is used only if there is no specifically
> named maintainer.

Many initial contributors of drivers and such put
their name in MAINTAINERS and stop soon after.

There are many "MAINTAINERS" that are inactive and
no longer contributors.

I suppose another change might be to cc signers when
the listed MAINTAINER has no sign-offs.

2010-09-26 01:04:18

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 05:50:43PM -0700, Joe Perches wrote:
> The reason get_maintainers by default cc'd signers is mostly
> historical. The file pattern coverage in MAINTAINERS when
> it was added wasn't very good, so signers were always added.
> It was also the Linus' preferred mechanism to find those
> "who really do the work".
>
> http://lkml.org/lkml/2007/8/14/276

Yeah, but Linus said "subdirectory or file". He was also assuming
some human intelligence. In the case of a filesystem, you should use
who has done most of the work in the *subdirectory*, and not on a
file-by-file basis. The brokeness is trying to do this in blind and
stupid script, that can't understand the difference between when you
should use subdirectory or a file.

This is what caused the completely insane result for fs/ext4/acl.c.
There have only been three commits in the past year, and they have all
been random folks who were doing random fixups --- many of which might
be stylistic cleanups. That's not "the people who really do the
work". It's just "the people who happened to do some random cleanups
on a file that happens not to change much". But the stupid thing is
trying to do it on a file-by-file basis in the first place, when for
something like fs/ext4, it really should be done on a subdirectory
basis.

That, BTW, is also my biggest complaint about checkpatch.pl. If it's
used by newbies who want to get warned about obvious things, that's
fine. If it's used by maintainers as an automated way to catch nits,
that's also fine. Maintainers are experts who know when it's OK to
disregard flase positives.

What really annoys me is newbies who use checkpatch.pl in its --file
mode, and then assume that every single warning is a deadly bug that
much be patched. Scripts by definitions are stupid, and don't
substitute for thinking. checkpatch.pl at least as the excuse that it
has some valid non-stupid uses. But I'm not convinced
get_maintainers.pl has the same excuse.

I at least never use it. I'll look through the MAINTAINERS file by
hand, or I'll use git log by hand, and let my human intelligence
figure out whether or not the patches that are turned up constitute
"those that do real work", or are bullshit checkpatch.pl cleanup
patches. Training people to use a script that by defintion can't be
smart enough to make these distinction ultimately is a huge disservice
to newbies (and experts won't use get_maintinaer.pl anyway, because
they will want to know the context).

- Ted

2010-09-26 01:32:29

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 21:04 -0400, Ted Ts'o wrote:
> But the stupid thing is
> trying to do it on a file-by-file basis in the first place, when for
> something like fs/ext4, it really should be done on a subdirectory
> basis.

That's not true at all.

If there's a listed maintainer linked to a pattern
like "F: fs/ext4/", then all files in that
directory and any subdirectories are "maintained".

If a maintainer wants to "exclude" particular files,
or subdirectories, that's possible with the "X:" tag.

> What really annoys me is newbies who use checkpatch.pl in its --file
> mode, and then assume that every single warning is a deadly bug that
> much be patched.

Perhaps checkpatch.pl should print some "don't submit
patches for stupid stuff" when used with -f.

> I at least never use it.

Nor are you required to.

Tool use is optional. I don't care if you carve
patches with a mallet onto stone tablets and send
them by swarms of carrier pigeon to Linus so can
reuse the stone to build an actual castle.

cheers, Joe

2010-09-26 01:53:28

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 06:32:26PM -0700, Joe Perches wrote:
> On Sat, 2010-09-25 at 21:04 -0400, Ted Ts'o wrote:
> > But the stupid thing is
> > trying to do it on a file-by-file basis in the first place, when for
> > something like fs/ext4, it really should be done on a subdirectory
> > basis.
>
> That's not true at all.

No, it *is* true. Someone with brains, as supposed to a stupid
script, would know that fs/ext4 should be treated as a unit. And
there *is* a F: fs/ext4 in the MAINTAINERS script.

Yet the "git-fallback" code still persisted in analyzing fs/ext4/acl.c
as a file by itself, and not as a subdirectory. That's WRONG. That's
not what Linus was telling you to do, if you're going to use that
e-mail of his as an excuse.

I'm glad you're now turning it off (at least by default) if there is a
MAINTAINER entry, but that code (disabled or not) is broken as it is.
You really need human intelligence to know whether to do things
file-by-file, or directory-by-directory. And if you can't figure it
out on your own, then the script shouldn't even try, or give a huge
warning that it's madly guessing and may be totally incorrect about
who you're telling the newbie to spam with their bug report.

> > I at least never use it.
>
> Nor are you required to.
>
> Tool use is optional. I don't care if you carve
> patches with a mallet onto stone tablets and send
> them by swarms of carrier pigeon to Linus so can
> reuse the stone to build an actual castle.

Yes, but the newbies don't know that they shouldn't use it, becuase it
can be wrong. And training them not to use their God-given brains,
instead of using a stupid script, is what I'm objecting to. That's
why I said, I'm not sure that get_maintainers.pl has an excuse for
existing. At least checkpatch.pl has some valid uses, even if it is
occasionaly abused.

I don't believe get_maintainers.pl does have legitmate use, since it's
really not that hard to look up something in MAINTAINERS, and if it's
not there, some real human judgement is needed, and not hueristic
guessing --- or at the very least, the script needs to warn that it's
guessing, and maybe explain to the user in detail why it's making the
guesses that it's making, so the user has a chance of understanding
why it might be completely wrong.

- Ted

2010-09-26 02:03:11

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 21:53 -0400, Ted Ts'o wrote:
> On Sat, Sep 25, 2010 at 06:32:26PM -0700, Joe Perches wrote:
> > On Sat, 2010-09-25 at 21:04 -0400, Ted Ts'o wrote:
> > > But the stupid thing is
> > > trying to do it on a file-by-file basis in the first place, when for
> > > something like fs/ext4, it really should be done on a subdirectory
> > > basis.
> > That's not true at all.
> No, it *is* true. Someone with brains, as supposed to a stupid
> script, would know that fs/ext4 should be treated as a unit. And
> there *is* a F: fs/ext4 in the MAINTAINERS script.

When you define "it" that way, not as any simple
file pattern match, but as a control for what
"git log -- path"
to inspect, it's quite feasible to use the pattern
match rather than the file name.

So, thanks, that's a good suggestion.

> I don't believe get_maintainers.pl does have legitmate use, since it's
> really not that hard to look up something in MAINTAINERS, and if it's
> not there, some real human judgement is needed, and not hueristic
> guessing --- or at the very least, the script needs to warn that it's
> guessing, and maybe explain to the user in detail why it's making the
> guesses that it's making, so the user has a chance of understanding
> why it might be completely wrong.

Multiple warning labels on tools tend to get ignored.
People that use tools without training tend to get injured.

cheers, Joe

2010-09-26 02:10:40

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 07:03:11PM -0700, Joe Perches wrote:
>
> When you define "it" that way, not as any simple
> file pattern match, but as a control for what
> "git log -- path"
> to inspect, it's quite feasible to use the pattern
> match rather than the file name.
>
> So, thanks, that's a good suggestion.

Don't make it an option, though. If you must use hueristics, then at
least *try* to make the hueristics smarter. If the file name falls
into certain patterns, such as:

fs/*/*.[ch]
drivers/scsi/*/*.[ch]
drivers/net/*/*.[ch]

etc., then you really should be doing the analysis by subdirectory,
and not by file.

> > I don't believe get_maintainers.pl does have legitmate use, since it's
> > really not that hard to look up something in MAINTAINERS, and if it's
> > not there, some real human judgement is needed, and not hueristic
> > guessing --- or at the very least, the script needs to warn that it's
> > guessing, and maybe explain to the user in detail why it's making the
> > guesses that it's making, so the user has a chance of understanding
> > why it might be completely wrong.
>
> Multiple warning labels on tools tend to get ignored.
> People that use tools without training tend to get injured.

There's something very simple you can do that would help. Don't make
it easy for people to do

mail $(./get_maintainer.pl) < annoying-e-mail

Instead have get_maintainer.pl always be very verbose, and have it
*explain* what it is doing, so that in order for someone to use the
damn thing, they have to read the text while they are finding the
e-mail address to cut and paste out of its output.

I really dislike training people that using dumb scripts is a good
idea. Tools like that just plain shouldn't exist, if they are that
subject to error. If you can clean up the tool to the point where the
error rate is manageable, maybe, perhaps.

But what was shipped as part of 2.6.35 was really, really, *REALLY*
bad. It makes me wonder how much testing someone did with the
hueristics before turning it loose for the newbies to use.

- Ted

2010-09-26 02:21:44

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

P.S. This is also completely insane:

--git-min-percent => minimum percentage of commits required (default: 5)

Anyone who has 5% of the commits should be considered a maintainer?!?

- Ted

2010-09-26 02:29:54

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 22:10 -0400, Ted Ts'o wrote:
> On Sat, Sep 25, 2010 at 07:03:11PM -0700, Joe Perches wrote:
> > When you define "it" that way, not as any simple
> > file pattern match, but as a control for what
> > "git log -- path"
> > to inspect, it's quite feasible to use the pattern
> > match rather than the file name.
> > So, thanks, that's a good suggestion.
> Don't make it an option, though. If you must use hueristics, then at
> least *try* to make the hueristics smarter. If the file name falls
> into certain patterns, such as:
> fs/*/*.[ch]
> drivers/scsi/*/*.[ch]
> drivers/net/*/*.[ch]
> etc., then you really should be doing the analysis by subdirectory,
> and not by file.

I think if there's an exact pattern "depth" match, then
git history should be searched by that subdirectory.

So for example:
F: drivers/scsi/megaraid/

A lookup for drivers/scsi/megaraid/mega_common.h (same depth,
search all commits in drivers/scsi/megaraid) now shows:

$ ./scripts/get_maintainer.pl -f drivers/scsi/megaraid/mega_common.h --rolestats --git
Neela Syam Kolli <[email protected]> (maintainer:MEGARAID SCSI DRI...)
"James E.J. Bottomley" <[email protected]> (maintainer:SCSI SUBSYSTEM,commit_signer:20/27=74%)
[email protected] (open list:MEGARAID SCSI DRI...)
[email protected] (open list)

> But what was shipped as part of 2.6.35 was really, really, *REALLY*
> bad. It makes me wonder how much testing someone did with the
> hueristics before turning it loose for the newbies to use.

A fair bit with all public discussions.

It's been shipped more or less that way since 2.6.30
beginning in April, 2009.

I get annoyed when people suggest that the maintainer
of that silly script isn't responsive.

2010-09-26 02:45:32

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, 2010-09-25 at 22:21 -0400, Ted Ts'o wrote:
> P.S. This is also completely insane:
> --git-min-percent => minimum percentage of commits required (default: 5)
> Anyone who has 5% of the commits should be considered a maintainer?!?

Probably --git-min-signatures=1 isn't high enough either.

I have no issue changing those to whatever is appropriate.

I believe the only person to comment on the % before
was Mark Brown, who might have suggested that number.

There've always been tradeoffs between activity on the file,
direct maintainership and reasonable accuracy of patterns.

Today, patterns are mostly reasonable, except for arch/arm/.

2010-09-26 06:11:32

[permalink] [raw]

Subject: Re: [PATCH 02/10] Fs: ext4: acl.h: whitespace cleanup

Tracey Dent <[email protected]> writes:

> --- a/fs/ext4/acl.h
> +++ b/fs/ext4/acl.h
> @@ -1,7 +1,7 @@
> /*
> - File: fs/ext4/acl.h
> + File: fs/ext4/acl.h
>
> - (C) 2001 Andreas Gruenbacher, <[email protected]>
> + (C) 2001 Andreas Gruenbacher, <[email protected]>
> */

Doesn't look like an improvement to me.
--
Krzysztof Halasax

2010-09-26 06:23:25

[permalink] [raw]

Subject: Re: [PATCH 05/10] Fs: ext4: ext4: cleaned up the file with checkpatch.pl

Tracey Dent <[email protected]> writes:

> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -233,8 +233,7 @@ typedef struct ext4_io_end {
> /*
> * Structure of a blocks group descriptor
> */
> -struct ext4_group_desc
> -{
> +struct ext4_group_desc {
> __le32 bg_block_bitmap_lo; /* Blocks bitmap block */
> __le32 bg_inode_bitmap_lo; /* Inodes bitmap block */

Putting a TAB instead of SPACE before the '{' isn't common here, is it?

> @@ -513,8 +512,8 @@ struct ext4_new_group_data {
> #define EXT4_IOC_GROUP_EXTEND _IOW('f', 7, unsigned long)
> #define EXT4_IOC_GROUP_ADD _IOW('f', 8, struct ext4_new_group_input)
> #define EXT4_IOC_MIGRATE _IO('f', 9)
> - /* note ioctl 10 reserved for an early version of the FIEMAP ioctl */
> - /* note ioctl 11 reserved for filesystem-independent FIEMAP ioctl */
> + /* note ioctl 10 reserved for an early version of the FIEMAP ioctl */
> + /* note ioctl 11 reserved for filesystem-independent FIEMAP ioctl */
> #define EXT4_IOC_ALLOC_DA_BLKS _IO('f', 12)

Doesn't look like an improvement to me.

> static inline __le32 ext4_encode_extra_time(struct timespec *time)
> {
> - return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
> - (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
> - ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));
> + return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
> + (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) |
> + ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK));

Wrong level of indentation.

> static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra)
> {
> - if (sizeof(time->tv_sec) > 4)
> + if (sizeof(time->tv_sec) > 4)

Ditto.
--
Krzysztof Halasa

2010-09-26 06:36:46

[permalink] [raw]

Subject: Re: [PATCH 10/10] Fs: ext4: namei: fixed file of checkpatch/pl warnings and errors

Tracey Dent <[email protected]> writes:

> + printk("%s%3u:%03u hash %8x/%8x ", levels ? "":" ", i, block, hash, range);
> + if (!(bh = ext4_bread(NULL,dir, block, 0,&err))) continue;

Put a SPACE after the comma. "continue" should usually be on the next
line. It doesn't make sense to fix the formatting only partially.

BTW I wouldn't trust checkpatch so blindly. Also keep in mind checkpatch
is only a tool and that maintainers have full authority to ignore the
problems (or "problems") it spews.
And I would merge all the changes (which make sense) and send them in
one patch - that should increase the odds of it actually being applied.
To be honest, fixing the style isn't an easy job.
--
Krzysztof Halasa

2010-09-26 18:23:42

[permalink] [raw]

Subject: Re: [PATCH 10/10] Fs: ext4: namei: fixed file of checkpatch/pl warnings and errors

On Sun, Sep 26, 2010 at 08:36:34AM +0200, Krzysztof Halasa wrote:
>
> BTW I wouldn't trust checkpatch so blindly. Also keep in mind checkpatch
> is only a tool and that maintainers have full authority to ignore the
> problems (or "problems") it spews.

I'm beginning to think we need to have an entry in the kernel newbie's
FAQ warning people that the output of various scripts such as
checkpatch and get_maintainer are not authoratative, and are
hueristics intended to be supplemented by human intelligence.

- TEd

2010-09-26 18:35:41

by Davidlohr Bueso

[permalink] [raw]

Subject: Re: [PATCH 10/10] Fs: ext4: namei: fixed file of checkpatch/pl warnings and errors

On Sun, 2010-09-26 at 14:23 -0400, Ted Ts'o wrote:
> On Sun, Sep 26, 2010 at 08:36:34AM +0200, Krzysztof Halasa wrote:
> >
> > BTW I wouldn't trust checkpatch so blindly. Also keep in mind checkpatch
> > is only a tool and that maintainers have full authority to ignore the
> > problems (or "problems") it spews.
>
> I'm beginning to think we need to have an entry in the kernel newbie's
> FAQ warning people that the output of various scripts such as
> checkpatch and get_maintainer are not authoratative, and are
> hueristics intended to be supplemented by human intelligence.

It would actually be quite useful to add a warning in kernelnewbies
about this, some people tend to get too carried away :)

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

2010-09-26 19:17:23

[permalink] [raw]

Subject: Re: [PATCH 10/10] Fs: ext4: namei: fixed file of checkpatch/pl warnings and errors

Ted Ts'o <[email protected]> writes:

> I'm beginning to think we need to have an entry in the kernel newbie's
> FAQ warning people that the output of various scripts such as
> checkpatch and get_maintainer are not authoratative, and are
> hueristics intended to be supplemented by human intelligence.

Definitely.

Current situation creates some false impressions, leaving the newcomers
confused (at best). Net effect = loss.
--
Krzysztof Halasa

2010-09-26 20:07:17

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH 01/10] Fs: ext4: acl.c: fixed indent issue

On Sat, Sep 25, 2010 at 05:06:19PM -0700, Joe Perches wrote:
> On Sun, 2010-09-26 at 01:56 +0200, Christoph Hellwig wrote:
> > On Sat, Sep 25, 2010 at 07:53:02PM -0400, T Dent wrote:
> > > On 9/25/10, Christoph Hellwig <[email protected]> wrote:
> > > > Please don't Cc me on ext4 patches, thanks.
> > > Ok.. i just used the get maintainer script and your name was on it.
> > The script is broken but the maintainer refuses to fix it.
>
> That'd be false.
>
> https://patchwork.kernel.org/patch/185352/

Thanks for changing your opinion after the last discussion and fixing
the worst fallout. Can you please prod Andrew to sned it on to Linus
ASAP?

2010-09-26 22:17:05