[PATCH 0/9] bfs: assorted cleanups

[PATCH 0/9] bfs: assorted cleanups

[Dmitri Vorobiev]

Hello Adrian,

Last time when I had sent some BFS bugfixes to the maintainer of
this filesystem driver, he did not respond, and Andrew Morton
had to take care of those.

For this reason, and because the patches in this little patch bomb
are trivial, I am sending this to you in the hope that these can
be pushed upstream when the next merge window opens.

In case you'll be wondering why I need BFS: I teach system programming
and operating systems at the premises of the Moscow State University.
The BFS code is used as an example of a simple filesystem driver
implementation in Linux. This hopefully explains why I want to have
clean code in here :)

Some checkpatch.pl stats follow.

Before:

----------------------------------------
|         | errors | warnings | checks |
----------------------------------------
| bfs.h   |   2    |    0     |   0    |
----------------------------------------
| dir.c   |   7    |    1     |   4    |
----------------------------------------
| file.c  |   6    |    0     |   2    |
----------------------------------------
| inode.c |   11   |    0     |   3    |
----------------------------------------

After:

----------------------------------------
|         | errors | warnings | checks |
----------------------------------------
| bfs.h   |   0    |    0     |   0    |
----------------------------------------
| dir.c   |   0    |    0     |   0    |
----------------------------------------
| file.c  |   0    |    0     |   0    |
----------------------------------------
| inode.c |   0    |    0     |   0    |
----------------------------------------

Please consider.

Thanks,

Dmitri Vorobiev

[PATCH 1/9] bfs: remove a useless variable

[Dmitri Vorobiev]

In the bfs_fill_super() routine, the "sblock" variable is declared
and assigned a value. However, this value is never used. This trivial
patch removes this useless variable.

This was compile-tested by building the bfs driver both as a module
and as a part of the kernel proper. Both build finished successfully.

Run time test was performed using a BFS image and verifying that the
filesystem remained functional after the change.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/inode.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index a64a71d..2284657 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -368,7 +368,7 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 		struct bfs_inode *di;
 		int block = (i - BFS_ROOT_INO) / BFS_INODES_PER_BLOCK + 1;
 		int off = (i - BFS_ROOT_INO) % BFS_INODES_PER_BLOCK;
-		unsigned long sblock, eblock;
+		unsigned long eblock;
 
 		if (!off) {
 			brelse(bh);
@@ -387,7 +387,6 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 		set_bit(i, info->si_imap);
 		info->si_freeb -= BFS_FILEBLOCKS(di);
 
-		sblock =  le32_to_cpu(di->i_sblock);
 		eblock =  le32_to_cpu(di->i_eblock);
 		if (eblock > info->si_lf_eblk)
 			info->si_lf_eblk = eblock;
-- 
1.5.3

[PATCH 2/9] bfs: coding style cleanup in fs/bfs/inode.c

[Dmitri Vorobiev]

This patch cleans up errors found by checkpatch.pl.

Before the patch:

$ ./scripts/checkpatch.pl --file fs/bfs/inode.c  | grep total
total: 11 errors, 0 warnings, 445 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file fs/bfs/inode.c  | grep total
total: 0 errors, 0 warnings, 446 lines checked

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/inode.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 2284657..5191990 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -90,12 +90,12 @@ static void bfs_read_inode(struct inode *inode)
 static int bfs_write_inode(struct inode *inode, int unused)
 {
 	unsigned int ino = (u16)inode->i_ino;
-        unsigned long i_sblock;
+	unsigned long i_sblock;
 	struct bfs_inode *di;
 	struct buffer_head *bh;
 	int block, off;
 
-        dprintf("ino=%08x\n", ino);
+	dprintf("ino=%08x\n", ino);
 
 	if ((ino < BFS_ROOT_INO) || (ino > BFS_SB(inode->i_sb)->si_lasti)) {
 		printf("Bad inode number %s:%08x\n", inode->i_sb->s_id, ino);
@@ -128,7 +128,7 @@ static int bfs_write_inode(struct inode *inode, int unused)
 	di->i_atime = cpu_to_le32(inode->i_atime.tv_sec);
 	di->i_mtime = cpu_to_le32(inode->i_mtime.tv_sec);
 	di->i_ctime = cpu_to_le32(inode->i_ctime.tv_sec);
-        i_sblock = BFS_I(inode)->i_sblock;
+	i_sblock = BFS_I(inode)->i_sblock;
 	di->i_sblock = cpu_to_le32(i_sblock);
 	di->i_eblock = cpu_to_le32(BFS_I(inode)->i_eblock);
 	di->i_eoffset = cpu_to_le32(i_sblock * BFS_BSIZE + inode->i_size - 1);
@@ -157,7 +157,7 @@ static void bfs_delete_inode(struct inode *inode)
 		printf("invalid ino=%08lx\n", ino);
 		return;
 	}
-	
+
 	inode->i_size = 0;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
 	lock_kernel();
@@ -177,13 +177,13 @@ static void bfs_delete_inode(struct inode *inode)
 	mark_buffer_dirty(bh);
 	brelse(bh);
 
-        if (bi->i_dsk_ino) {
+	if (bi->i_dsk_ino) {
 		if (bi->i_sblock)
 			info->si_freeb += bi->i_eblock + 1 - bi->i_sblock;
 		info->si_freei++;
 		clear_bit(ino, info->si_imap);
 		dump_imap("delete_inode", s);
-        }
+	}
 
 	/*
 	 * If this was the last file, make the previous block
@@ -293,7 +293,8 @@ void dump_imap(const char *prefix, struct super_block *s)
 	if (!tmpbuf)
 		return;
 	for (i = BFS_SB(s)->si_lasti; i >= 0; i--) {
-		if (i > PAGE_SIZE - 100) break;
+		if (i > PAGE_SIZE - 100)
+			break;
 		if (test_bit(i, BFS_SB(s)->si_imap))
 			strcat(tmpbuf, "1");
 		else
@@ -321,12 +322,12 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 	sb_set_blocksize(s, BFS_BSIZE);
 
 	bh = sb_bread(s, 0);
-	if(!bh)
+	if (!bh)
 		goto out;
 	bfs_sb = (struct bfs_super_block *)bh->b_data;
 	if (le32_to_cpu(bfs_sb->s_magic) != BFS_MAGIC) {
 		if (!silent)
-			printf("No BFS filesystem on %s (magic=%08x)\n", 
+			printf("No BFS filesystem on %s (magic=%08x)\n",
 				s->s_id,  le32_to_cpu(bfs_sb->s_magic));
 		goto out;
 	}
@@ -395,7 +396,7 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 	if (!(s->s_flags & MS_RDONLY)) {
 		mark_buffer_dirty(info->si_sbh);
 		s->s_dirt = 1;
-	} 
+	}
 	dump_imap("read_super", s);
 	return 0;
 
@@ -425,7 +426,7 @@ static int __init init_bfs_fs(void)
 	int err = init_inodecache();
 	if (err)
 		goto out1;
-        err = register_filesystem(&bfs_fs_type);
+	err = register_filesystem(&bfs_fs_type);
 	if (err)
 		goto out;
 	return 0;
-- 
1.5.3

[PATCH 3/9] bfs: coding style cleanup in fs/bfs/bfs.h

[Dmitri Vorobiev]

Clean up errors found by checkpatch.pl.

Before the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/bfs.h | grep total
total: 2 errors, 0 warnings, 57 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/bfs.h | grep total
total: 0 errors, 0 warnings, 57 lines checked

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/bfs.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
index ac7a8b1..090b96e 100644
--- a/fs/bfs/bfs.h
+++ b/fs/bfs/bfs.h
@@ -16,8 +16,8 @@ struct bfs_sb_info {
 	unsigned long si_freei;
 	unsigned long si_lf_eblk;
 	unsigned long si_lasti;
-	unsigned long * si_imap;
-	struct buffer_head * si_sbh;		/* buffer header w/superblock */
+	unsigned long *si_imap;
+	struct buffer_head *si_sbh;		/* buffer header w/superblock */
 };
 
 /*
-- 
1.5.3

[PATCH 4/9] bfs: coding style cleanup in fs/bfs/dir.c

[Dmitri Vorobiev]

Clean up errors found by checkpatch.pl.

Before the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/dir.c | grep total
total: 7 errors, 1 warnings, 370 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/dir.c | grep total
total: 0 errors, 1 warnings, 370 lines checked

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/dir.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 1fd056d..5462a5b 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -72,7 +72,7 @@ static int bfs_readdir(struct file *f, void *dirent, filldir_t filldir)
 	}
 
 	unlock_kernel();
-	return 0;	
+	return 0;
 }
 
 const struct file_operations bfs_dir_operations = {
@@ -117,7 +117,7 @@ static int bfs_create(struct inode *dir, struct dentry *dentry, int mode,
 	BFS_I(inode)->i_sblock = 0;
 	BFS_I(inode)->i_eblock = 0;
 	insert_inode_hash(inode);
-        mark_inode_dirty(inode);
+	mark_inode_dirty(inode);
 	dump_imap("create", s);
 
 	err = bfs_add_entry(dir, dentry->d_name.name, dentry->d_name.len,
@@ -228,8 +228,8 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		return -EINVAL;
 
 	lock_kernel();
-	old_bh = bfs_find_entry(old_dir, 
-				old_dentry->d_name.name, 
+	old_bh = bfs_find_entry(old_dir,
+				old_dentry->d_name.name,
 				old_dentry->d_name.len, &old_de);
 
 	if (!old_bh || (le16_to_cpu(old_de->ino) != old_inode->i_ino))
@@ -237,8 +237,8 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	error = -EPERM;
 	new_inode = new_dentry->d_inode;
-	new_bh = bfs_find_entry(new_dir, 
-				new_dentry->d_name.name, 
+	new_bh = bfs_find_entry(new_dir,
+				new_dentry->d_name.name,
 				new_dentry->d_name.len, &new_de);
 
 	if (new_bh && !new_inode) {
@@ -246,7 +246,7 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		new_bh = NULL;
 	}
 	if (!new_bh) {
-		error = bfs_add_entry(new_dir, 
+		error = bfs_add_entry(new_dir,
 					new_dentry->d_name.name,
 					new_dentry->d_name.len,
 					old_inode->i_ino);
-- 
1.5.3

[PATCH 5/9] bfs: move function prototype to the proper header file

[Dmitri Vorobiev]

The dump_imap() routine is defined in bs/bfs/inode.c and used both in
the same file and in fs/bfs/dir.c. This patch adds an extern function
declaration to the private bfs.h header file.

The effect is that one warning issued by checkpatch.pl is gone.

Before the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/dir.c | grep total
total: 0 errors, 1 warnings, 370 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/dir.c | grep total
total: 0 errors, 0 warnings, 368 lines checked

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/bfs.h   |    3 +++
 fs/bfs/dir.c   |    2 --
 fs/bfs/inode.c |    2 --
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
index 090b96e..ecc74bb 100644
--- a/fs/bfs/bfs.h
+++ b/fs/bfs/bfs.h
@@ -54,4 +54,7 @@ extern const struct address_space_operations bfs_aops;
 extern const struct inode_operations bfs_dir_inops;
 extern const struct file_operations bfs_dir_operations;
 
+/* inode.c */
+extern void dump_imap(const char *, struct super_block *);
+
 #endif /* _FS_BFS_BFS_H */
diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 5462a5b..2964505 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -81,8 +81,6 @@ const struct file_operations bfs_dir_operations = {
 	.fsync		= file_fsync,
 };
 
-extern void dump_imap(const char *, struct super_block *);
-
 static int bfs_create(struct inode *dir, struct dentry *dentry, int mode,
 						struct nameidata *nd)
 {
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 5191990..91d5686 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -30,8 +30,6 @@ MODULE_LICENSE("GPL");
 #define dprintf(x...)
 #endif
 
-void dump_imap(const char *prefix, struct super_block *s);
-
 static void bfs_read_inode(struct inode *inode)
 {
 	unsigned long ino = inode->i_ino;
-- 
1.5.3

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Heikki Orsila]

On Fri, Jan 25, 2008 at 01:32:04AM +0300, Dmitri Vorobiev wrote:
> diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
> index 090b96e..ecc74bb 100644
> --- a/fs/bfs/bfs.h
> +++ b/fs/bfs/bfs.h
...
> +/* inode.c */
> +extern void dump_imap(const char *, struct super_block *);
> +

Functions should not be externed, remove extern keyword.

-- 
Heikki Orsila			Barbie's law:
heikki.orsila@iki.fi		"Math is hard, let's go shopping!"
http://www.iki.fi/shd

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Tigran Aivazian]

On Fri, 25 Jan 2008, Heikki Orsila wrote:
> On Fri, Jan 25, 2008 at 01:32:04AM +0300, Dmitri Vorobiev wrote:
>> diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
>> index 090b96e..ecc74bb 100644
>> --- a/fs/bfs/bfs.h
>> +++ b/fs/bfs/bfs.h
> ...
>> +/* inode.c */
>> +extern void dump_imap(const char *, struct super_block *);
>> +
>
> Functions should not be externed, remove extern keyword.

why not?

In (roughly, because ^extern pattern is not ideal) 3959 cases only in 
include/linux/*h they are:

$ grep ^extern include/linux/*h | wc -l
3959

Kind regards
Tigran

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Tigran Aivazian]

Ooops, I didn't look at the _name_ of the function, i.e. it being 
dump_imap(), an internal helper --- of course it shouldn't be extern'd you 
are right :)

On Thu, 24 Jan 2008, Tigran Aivazian wrote:

> On Fri, 25 Jan 2008, Heikki Orsila wrote:
>> On Fri, Jan 25, 2008 at 01:32:04AM +0300, Dmitri Vorobiev wrote:
>>> diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
>>> index 090b96e..ecc74bb 100644
>>> --- a/fs/bfs/bfs.h
>>> +++ b/fs/bfs/bfs.h
>> ...
>>> +/* inode.c */
>>> +extern void dump_imap(const char *, struct super_block *);
>>> +
>> 
>> Functions should not be externed, remove extern keyword.
>
> why not?
>
> In (roughly, because ^extern pattern is not ideal) 3959 cases only in 
> include/linux/*h they are:
>
> $ grep ^extern include/linux/*h | wc -l
> 3959
>
> Kind regards
> Tigran
>

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Dmitri Vorobiev]

Heikki Orsila пишет:
> On Fri, Jan 25, 2008 at 01:32:04AM +0300, Dmitri Vorobiev wrote:
>> diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
>> index 090b96e..ecc74bb 100644
>> --- a/fs/bfs/bfs.h
>> +++ b/fs/bfs/bfs.h
> ...
>> +/* inode.c */
>> +extern void dump_imap(const char *, struct super_block *);
>> +
> 
> Functions should not be externed, remove extern keyword.
> 

Care to explain why?

Following is an explanation why the contrary is probably true:

1) We have lots of precedents in existing code:

dmvo@cipher:~/Projects/misc/linux$ git-grep 'extern void' include | wc -l
5523
dmvo@cipher:~/Projects/misc/linux$

2) Linus' Coding style does not mandate what you requested.

3) The checkpatch.pl did not complain at this particular patch.

Thanks,

Dmitri

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Tigran Aivazian]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 761 bytes --]

On Fri, 25 Jan 2008, Dmitri Vorobiev wrote:

> Heikki Orsila пишет:
>>> +extern void dump_imap(const char *, struct super_block *);
>>> +
>>
>> Functions should not be externed, remove extern keyword.
>>
>
> Care to explain why?

because dump_imap() is just a BFS' internal helper (for debugging purposes 
only btw) to dump the inode map via printk. Why should it be moved into 
the header, i.e. where one expects to see things potentially visible by 
the rest of the kernel?

> 3) The checkpatch.pl did not complain at this particular patch.

maybe this script should be simplified to not complain at things like 
that, which are best left to the author of the code to decide which 
declaration should be where?

Kind regards
Tigran

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Dmitri Vorobiev]

Tigran Aivazian пишет:
> On Fri, 25 Jan 2008, Dmitri Vorobiev wrote:
> 
>> Heikki Orsila пишет:
>>>> +extern void dump_imap(const char *, struct super_block *);
>>>> +
>>>
>>> Functions should not be externed, remove extern keyword.
>>>
>>
>> Care to explain why?
> 
> because dump_imap() is just a BFS' internal helper (for debugging
> purposes only btw) to dump the inode map via printk. Why should it be
> moved into the header, i.e. where one expects to see things potentially
> visible by the rest of the kernel?
>

Thanks, Tigran.

Please find below the corrected version. Compilation test passed successfully.

Dmitri
 
======

The dump_imap() routine is defined in bs/bfs/inode.c and used both in
the same file and in fs/bfs/dir.c. This patch adds an extern function
declaration to the private bfs.h header file.

The effect is that one warning issued by checkpatch.pl is gone.

Before the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/dir.c | grep total
total: 0 errors, 1 warnings, 370 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/dir.c | grep total
total: 0 errors, 0 warnings, 368 lines checked

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
diff --git a/fs/bfs/bfs.h b/fs/bfs/bfs.h
index 090b96e..352804f 100644
--- a/fs/bfs/bfs.h
+++ b/fs/bfs/bfs.h
@@ -54,4 +54,7 @@ extern const struct address_space_operations bfs_aops;
 extern const struct inode_operations bfs_dir_inops;
 extern const struct file_operations bfs_dir_operations;
 
+/* inode.c */
+void dump_imap(const char *, struct super_block *);
+
 #endif /* _FS_BFS_BFS_H */
diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 5462a5b..2964505 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -81,8 +81,6 @@ const struct file_operations bfs_dir_operations = {
 	.fsync		= file_fsync,
 };
 
-extern void dump_imap(const char *, struct super_block *);
-
 static int bfs_create(struct inode *dir, struct dentry *dentry, int mode,
 						struct nameidata *nd)
 {
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 5191990..91d5686 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -30,8 +30,6 @@ MODULE_LICENSE("GPL");
 #define dprintf(x...)
 #endif
 
-void dump_imap(const char *prefix, struct super_block *s);
-
 static void bfs_read_inode(struct inode *inode)
 {
 	unsigned long ino = inode->i_ino;

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Adrian Bunk]

On Thu, Jan 24, 2008 at 11:22:21PM +0000, Tigran Aivazian wrote:
> On Fri, 25 Jan 2008, Dmitri Vorobiev wrote:
>
>> Heikki Orsila пишет:
>>>> +extern void dump_imap(const char *, struct super_block *);
>>>> +
>>>
>>> Functions should not be externed, remove extern keyword.
>>>
>>
>> Care to explain why?
>
> because dump_imap() is just a BFS' internal helper (for debugging 
> purposes only btw) to dump the inode map via printk. Why should it be 
> moved into the header, i.e. where one expects to see things potentially 
> visible by the rest of the kernel?
>...

fs/bfs/bfs.h is not visible to the rest of the kernel, it's the right 
place for bfs-internal code.

Whether there's an "extern" written is just a syntax thing with zero 
semantical implications. We tend to not write the "extern" in the 
kernel, but that's nothing cast in stone.

But prototypes really belong into header files - bugs in this area are 
rare, but when they occur they can cause nasty hard-to-debug bugs
(e.g. depending on the calling convention on the architecture calling 
the function can turn your stack into garbage), and with the prototypes 
in header files gcc can do type checks.

> Kind regards
> Tigran

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Dmitri Vorobiev]

Adrian Bunk пишет:
> On Thu, Jan 24, 2008 at 11:22:21PM +0000, Tigran Aivazian wrote:
>> On Fri, 25 Jan 2008, Dmitri Vorobiev wrote:
>>
>>> Heikki Orsila пишет:
>>>>> +extern void dump_imap(const char *, struct super_block *);
>>>>> +
>>>> Functions should not be externed, remove extern keyword.
>>>>
>>> Care to explain why?
>> because dump_imap() is just a BFS' internal helper (for debugging 
>> purposes only btw) to dump the inode map via printk. Why should it be 
>> moved into the header, i.e. where one expects to see things potentially 
>> visible by the rest of the kernel?
>> ...
> 
> fs/bfs/bfs.h is not visible to the rest of the kernel, it's the right 
> place for bfs-internal code.
> 
> Whether there's an "extern" written is just a syntax thing with zero 
> semantical implications. We tend to not write the "extern" in the 
> kernel, but that's nothing cast in stone.

Adrian, thanks for the feedback.

The next version of this patch series will not have the extern keyword
for this helper. I'll try to have the second version sent to trivial
by tomorrow.

Dmitri

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Heikki Orsila]

On Fri, Jan 25, 2008 at 02:13:00AM +0300, Dmitri Vorobiev wrote:
> Care to explain why?

Because functions are always external objects in C. I just verified that 
from K&R.

-- 
Heikki Orsila			Barbie's law:
heikki.orsila@iki.fi		"Math is hard, let's go shopping!"
http://www.iki.fi/shd

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Dmitri Vorobiev]

Heikki Orsila пишет:
> On Fri, Jan 25, 2008 at 02:13:00AM +0300, Dmitri Vorobiev wrote:
>> Care to explain why?
> 
> Because functions are always external objects in C. I just verified that 
> from K&R.
> 

Yes, I know that :)

The reasons behind me using this keyword were: 1) to keep the code symmetric;
2)-4) as explained elsewhere in this thread.

Anyways, you and Tigran have been convincing enough and the corrected patch
is there.

Thanks,

Dmitri

Re: [PATCH 5/9] bfs: move function prototype to the proper header file

[Kyle Moffett]

On Jan 24, 2008, at 18:13, Dmitri Vorobiev wrote:
> Heikki Orsila пишет:
>> On Fri, Jan 25, 2008 at 01:32:04AM +0300, Dmitri Vorobiev wrote:
>>> +/* inode.c */
>>> +extern void dump_imap(const char *, struct super_block *);
>>> +
>>
>> Functions should not be externed, remove extern keyword.
>
> Care to explain why?
>
> Following is an explanation why the contrary is probably true:
>
> 1) We have lots of precedents in existing code:
>
> dmvo@cipher:~/Projects/misc/linux$ git-grep 'extern void' include |  
> wc -l
> 5523
> dmvo@cipher:~/Projects/misc/linux$


The "extern" keyword on functions is *completely* redundant.

For C variables:
   Declaration:  extern int foo;
   Definition:   int foo;
   File-scoped:  static int foo;

For C functions:
   Declaration:  void foo(int x);
   Definition:   void foo(int x) { /*...body...*/ }
   File-scoped:  static void foo(int x) { /*...body...*/ }

The compiler will *allow* you to use "extern" on the function  
prototype, but the presence or absence of a function body is  
sufficiently obvious for it to determine whether the prototype is a  
declaration or a definition that the "extern" keyword is not required  
and therefore redundant.

For maximum readability and cleanliness I recommend that you leave off  
the "extern" on the function declarations; it makes the lines much  
longer without obvious gain.

Cheers,
Kyle Moffett

[PATCH 6/9] bfs: coding style cleanup in fs/bfs/file.c

[Dmitri Vorobiev]

Clean up errors found by checkpatch.pl.

Before the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/file.c | grep total
total: 6 errors, 0 warnings, 191 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file  fs/bfs/file.c | grep total
total: 0 errors, 0 warnings, 191 lines checked

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/file.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index b11e63e..f32b2a2 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -55,7 +55,7 @@ static int bfs_move_blocks(struct super_block *sb, unsigned long start,
 
 	dprintf("%08lx-%08lx->%08lx\n", start, end, where);
 	for (i = start; i <= end; i++)
-		if(bfs_move_block(i, where + i, sb)) {
+		if (bfs_move_block(i, where + i, sb)) {
 			dprintf("failed to move block %08lx -> %08lx\n", i,
 								where + i);
 			return -EIO;
@@ -77,7 +77,7 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 	if (!create) {
 		if (phys <= bi->i_eblock) {
 			dprintf("c=%d, b=%08lx, phys=%09lx (granted)\n",
-                                create, (unsigned long)block, phys);
+					create, (unsigned long)block, phys);
 			map_bh(bh_result, sb, phys);
 		}
 		return 0;
@@ -88,7 +88,7 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 	 * range of blocks allocated for this file, we can grant it.
 	 */
 	if (bi->i_sblock && (phys <= bi->i_eblock)) {
-		dprintf("c=%d, b=%08lx, phys=%08lx (interim block granted)\n", 
+		dprintf("c=%d, b=%08lx, phys=%08lx (interim block granted)\n",
 				create, (unsigned long)block, phys);
 		map_bh(bh_result, sb, phys);
 		return 0;
@@ -107,7 +107,7 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 	 * anywhere.
 	 */
 	if (bi->i_eblock == info->si_lf_eblk) {
-		dprintf("c=%d, b=%08lx, phys=%08lx (simple extension)\n", 
+		dprintf("c=%d, b=%08lx, phys=%08lx (simple extension)\n",
 				create, (unsigned long)block, phys);
 		map_bh(bh_result, sb, phys);
 		info->si_freeb -= phys - bi->i_eblock;
@@ -126,7 +126,7 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 	}
 
 	if (bi->i_sblock) {
-		err = bfs_move_blocks(inode->i_sb, bi->i_sblock, 
+		err = bfs_move_blocks(inode->i_sb, bi->i_sblock,
 						bi->i_eblock, phys);
 		if (err) {
 			dprintf("failed to move ino=%08lx -> fs corruption\n",
@@ -137,7 +137,7 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 		err = 0;
 
 	dprintf("c=%d, b=%08lx, phys=%08lx (moved)\n",
-                create, (unsigned long)block, phys);
+			create, (unsigned long)block, phys);
 	bi->i_sblock = phys;
 	phys += block;
 	info->si_lf_eblk = bi->i_eblock = phys;
-- 
1.5.3

[PATCH 7/9] bfs: coding style cleanup in include/linux/bfs_fs.h

[Dmitri Vorobiev]

Clean up errors found by checkpatch.pl.

Before the patch:

$ ./scripts/checkpatch.pl --file  include/linux/bfs_fs.h | grep total
total: 5 errors, 3 warnings, 80 lines checked

After the patch:

$ ./scripts/checkpatch.pl --file  include/linux/bfs_fs.h | grep total
total: 0 errors, 0 warnings, 83 lines checked

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 include/linux/bfs_fs.h |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/linux/bfs_fs.h b/include/linux/bfs_fs.h
index 8ed6dfd..d7b11a6 100644
--- a/include/linux/bfs_fs.h
+++ b/include/linux/bfs_fs.h
@@ -36,7 +36,7 @@ struct bfs_inode {
 	__u32 i_padding[4];
 };
 
-#define BFS_NAMELEN		14	
+#define BFS_NAMELEN		14
 #define BFS_DIRENT_SIZE		16
 #define BFS_DIRS_PER_BLOCK	32
 
@@ -61,20 +61,23 @@ struct bfs_super_block {
 
 
 #define BFS_OFF2INO(offset) \
-        ((((offset) - BFS_BSIZE) / sizeof(struct bfs_inode)) + BFS_ROOT_INO)
+	((((offset) - BFS_BSIZE) / sizeof(struct bfs_inode)) + BFS_ROOT_INO)
 
 #define BFS_INO2OFF(ino) \
 	((__u32)(((ino) - BFS_ROOT_INO) * sizeof(struct bfs_inode)) + BFS_BSIZE)
 #define BFS_NZFILESIZE(ip) \
-        ((le32_to_cpu((ip)->i_eoffset) + 1) -  le32_to_cpu((ip)->i_sblock) * BFS_BSIZE)
+	((le32_to_cpu((ip)->i_eoffset) + 1) - \
+	 le32_to_cpu((ip)->i_sblock) * BFS_BSIZE)
 
 #define BFS_FILESIZE(ip) \
-        ((ip)->i_sblock == 0 ? 0 : BFS_NZFILESIZE(ip))
+	((ip)->i_sblock == 0 ? 0 : BFS_NZFILESIZE(ip))
 
 #define BFS_FILEBLOCKS(ip) \
-        ((ip)->i_sblock == 0 ? 0 : (le32_to_cpu((ip)->i_eblock) + 1) -  le32_to_cpu((ip)->i_sblock))
+	((ip)->i_sblock == 0 ? 0 : (le32_to_cpu((ip)->i_eblock) + 1) - \
+	 le32_to_cpu((ip)->i_sblock))
 #define BFS_UNCLEAN(bfs_sb, sb)	\
-	((le32_to_cpu(bfs_sb->s_from) != -1) && (le32_to_cpu(bfs_sb->s_to) != -1) && !(sb->s_flags & MS_RDONLY))
-
+	((le32_to_cpu(bfs_sb->s_from) != -1) && \
+	(le32_to_cpu(bfs_sb->s_to) != -1) && \
+	!(sb->s_flags & MS_RDONLY))
 
 #endif	/* _LINUX_BFS_FS_H */
-- 
1.5.3

[PATCH 8/9] bfs: remove multiple assignments

[Dmitri Vorobiev]

The checkpatch.pl reported several warnings about multiple variable
assignments in the BFS driver sources. This trivial patch fixes these
warnings by giving each assignment its own line.

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/dir.c   |   13 +++++++++----
 fs/bfs/file.c  |    6 ++++--
 fs/bfs/inode.c |    7 +++++--
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/fs/bfs/dir.c b/fs/bfs/dir.c
index 2964505..94fed7a 100644
--- a/fs/bfs/dir.c
+++ b/fs/bfs/dir.c
@@ -104,7 +104,9 @@ static int bfs_create(struct inode *dir, struct dentry *dentry, int mode,
 	info->si_freei--;
 	inode->i_uid = current->fsuid;
 	inode->i_gid = (dir->i_mode & S_ISGID) ? dir->i_gid : current->fsgid;
-	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC;
+	inode->i_mtime = CURRENT_TIME_SEC;
+	inode->i_atime = CURRENT_TIME_SEC;
+	inode->i_ctime = CURRENT_TIME_SEC;
 	inode->i_blocks = 0;
 	inode->i_op = &bfs_file_inops;
 	inode->i_fop = &bfs_file_operations;
@@ -200,7 +202,8 @@ static int bfs_unlink(struct inode *dir, struct dentry *dentry)
 	}
 	de->ino = 0;
 	mark_buffer_dirty(bh);
-	dir->i_ctime = dir->i_mtime = CURRENT_TIME_SEC;
+	dir->i_ctime = CURRENT_TIME_SEC;
+	dir->i_mtime = CURRENT_TIME_SEC;
 	mark_inode_dirty(dir);
 	inode->i_ctime = dir->i_ctime;
 	inode_dec_link_count(inode);
@@ -220,7 +223,8 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	struct bfs_dirent *old_de, *new_de;
 	int error = -ENOENT;
 
-	old_bh = new_bh = NULL;
+	old_bh = NULL;
+	new_bh = NULL;
 	old_inode = old_dentry->d_inode;
 	if (S_ISDIR(old_inode->i_mode))
 		return -EINVAL;
@@ -252,7 +256,8 @@ static int bfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 			goto end_rename;
 	}
 	old_de->ino = 0;
-	old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME_SEC;
+	old_dir->i_ctime = CURRENT_TIME_SEC;
+	old_dir->i_mtime = CURRENT_TIME_SEC;
 	mark_inode_dirty(old_dir);
 	if (new_inode) {
 		new_inode->i_ctime = CURRENT_TIME_SEC;
diff --git a/fs/bfs/file.c b/fs/bfs/file.c
index f32b2a2..ab2fa66 100644
--- a/fs/bfs/file.c
+++ b/fs/bfs/file.c
@@ -111,7 +111,8 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 				create, (unsigned long)block, phys);
 		map_bh(bh_result, sb, phys);
 		info->si_freeb -= phys - bi->i_eblock;
-		info->si_lf_eblk = bi->i_eblock = phys;
+		info->si_lf_eblk = phys;
+		bi->i_eblock = phys;
 		mark_inode_dirty(inode);
 		mark_buffer_dirty(sbh);
 		err = 0;
@@ -140,7 +141,8 @@ static int bfs_get_block(struct inode *inode, sector_t block,
 			create, (unsigned long)block, phys);
 	bi->i_sblock = phys;
 	phys += block;
-	info->si_lf_eblk = bi->i_eblock = phys;
+	info->si_lf_eblk = phys;
+	bi->i_eblock = phys;
 
 	/*
 	 * This assumes nothing can write the inode back while we are here
diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 91d5686..7eefafb 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -157,7 +157,9 @@ static void bfs_delete_inode(struct inode *inode)
 	}
 
 	inode->i_size = 0;
-	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC;
+	inode->i_atime = CURRENT_TIME_SEC;
+	inode->i_mtime = CURRENT_TIME_SEC;
+	inode->i_ctime = CURRENT_TIME_SEC;
 	lock_kernel();
 	mark_inode_dirty(inode);
 
@@ -213,7 +215,8 @@ static int bfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_type = BFS_MAGIC;
 	buf->f_bsize = s->s_blocksize;
 	buf->f_blocks = info->si_blocks;
-	buf->f_bfree = buf->f_bavail = info->si_freeb;
+	buf->f_bfree = info->si_freeb;
+	buf->f_bavail = info->si_freeb;
 	buf->f_files = info->si_lasti + 1 - BFS_ROOT_INO;
 	buf->f_ffree = info->si_freei;
 	buf->f_fsid.val[0] = (u32)id;
-- 
1.5.3

[PATCH 9/9] bfs: use the proper header file for inclusion

[Dmitri Vorobiev]

The checkpatch.pl reported the following warning:

$ ./scripts/checkpatch.pl --strict --file fs/bfs/inode.c
CHECK: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>
+#include <asm/uaccess.h>

This patch fixes this warning.

No functional changes introduced.

This patch was compile-tested by building the BFS driver both
as a module and as a part of the kernel proper.

Signed-off-by: Dmitri Vorobiev <dmitri.vorobiev@gmail.com>
---
 fs/bfs/inode.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index 7eefafb..00c54fa 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -15,7 +15,7 @@
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <linux/vfs.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include "bfs.h"
 
 MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
-- 
1.5.3

Re: [PATCH 0/9] bfs: assorted cleanups

[Dmitri Vorobiev]

Dmitri Vorobiev пишет:
> Hello Adrian,
> 
> Last time when I had sent some BFS bugfixes to the maintainer of
> this filesystem driver, he did not respond, and Andrew Morton
> had to take care of those.
> 
> For this reason, and because the patches in this little patch bomb
> are trivial, I am sending this to you in the hope that these can
> be pushed upstream when the next merge window opens.

Adrian, please drop this version of the patches. I'll send you a new
one addressing the feedback and indicating the ack I received from the
driver maintainer in a private email.

Thanks,

Dmitri