: UDF code style conversion to kernel style

This patch converts UDF system coding style to kernel coding style. There is no error fixing - just code cleanup. I attempted to make the minimum changes as possible but the patch was growing and growing. So there is a lot of code to review and that could be painful and boring. I've it reviewed several times to assure that the patch does not break current UDF system... but I've only one pair of eyes.

Any comments, suggestions and swearing are welcome. But don't be expected for fast reply - I'm not mature kernel developer ;)

---

Andrew, this patch is over current -mm tree (I've assumed that my prev patches were already applied).

 fs/udf/balloc.c    |  485 +++++++++++---------------
 fs/udf/dir.c       |   92 ++---
 fs/udf/directory.c |  148 ++++-----
 fs/udf/ecma_167.h  |  166 +++------
 fs/udf/file.c      |   68 ++--
 fs/udf/fsync.c     |    4 +-
 fs/udf/ialloc.c    |   36 +-
 fs/udf/inode.c     |  991 ++++++++++++++++++++++------------------------------
 fs/udf/lowlevel.c  |   15 +-
 fs/udf/misc.c      |  113 +++----
 fs/udf/namei.c     |  512 +++++++++++----------------
 fs/udf/osta_udf.h  |   53 +--
 fs/udf/partition.c |  117 +++----
 fs/udf/super.c     |  773 +++++++++++++++++------------------------
 fs/udf/symlink.c   |   22 +-
 fs/udf/truncate.c  |  123 +++----
 fs/udf/udf_sb.h    |   20 +-
 fs/udf/udfdecl.h   |   12 +-
 fs/udf/udfend.h    |   18 +
 fs/udf/udftime.c   |   73 ++--
 fs/udf/unicode.c   |  237 +++++--------
 21 files changed, 1689 insertions(+), 2389 deletions(-)

index 6ded93e..6c67300 100644
@@ -32,6 +32,7 @@ static int udf_fsync_inode(struct inode *, int);
 int udf_fsync_file(struct file * file, struct dentry *dentry, int datasync)
 {
 	struct inode *inode = dentry->d_inode;
+
 	return udf_fsync_inode(inode, datasync);
 }
 

@@ -45,6 +46,7 @@ static int udf_fsync_inode(struct inode *inode, int datasync)
 	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
 		return err;
 
-	err |= udf_sync_inode (inode);
+	err |= udf_sync_inode(inode);
+
 	return err ? -EIO : 0;
 }

index 8206983..c022e94 100644
@@ -50,7 +50,7 @@ void udf_free_inode(struct inode * inode)
 		else
 			UDF_SB_LVIDIU(sb)->numFiles =
 				cpu_to_le32(le32_to_cpu(UDF_SB_LVIDIU(sb)->numFiles) - 1);
-		
+
 		mark_buffer_dirty(sbi->s_lvidbh);
 	}
 	mutex_unlock(&sbi->s_alloc_mutex);

@@ -68,8 +68,7 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 
 	inode = new_inode(sb);
 
-	if (!inode)
-	{
+	if (!inode) {
 		*err = -ENOMEM;
 		return NULL;
 	}

@@ -82,16 +81,14 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 	UDF_I_STRAT4096(inode) = 0;
 
 	block = udf_new_block(dir->i_sb, NULL, UDF_I_LOCATION(dir).partitionReferenceNum,
-		start, err);
-	if (*err)
-	{
+			      start, err);
+	if (*err) {
 		iput(inode);
 		return NULL;
 	}
 
 	mutex_lock(&sbi->s_alloc_mutex);
-	if (UDF_SB_LVIDBH(sb))
-	{
+	if (UDF_SB_LVIDBH(sb)) {
 		struct logicalVolHeaderDesc *lvhd;
 		uint64_t uniqueID;
 		lvhd = (struct logicalVolHeaderDesc *)(UDF_SB_LVID(sb)->logicalVolContentsUse);

@@ -109,14 +106,13 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 	}
 	inode->i_mode = mode;
 	inode->i_uid = current->fsuid;
-	if (dir->i_mode & S_ISGID)
-	{
+	if (dir->i_mode & S_ISGID) {
 		inode->i_gid = dir->i_gid;
 		if (S_ISDIR(mode))
 			mode |= S_ISGID;
-	}
-	else
+	} else {
 		inode->i_gid = current->fsgid;
+	}
 
 	UDF_I_LOCATION(inode).logicalBlockNum = block;
 	UDF_I_LOCATION(inode).partitionReferenceNum = UDF_I_LOCATION(dir).partitionReferenceNum;

@@ -125,17 +121,20 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 	UDF_I_LENEATTR(inode) = 0;
 	UDF_I_LENALLOC(inode) = 0;
 	UDF_I_USE(inode) = 0;
-	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_EXTENDED_FE))
-	{
+	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_EXTENDED_FE)) {
 		UDF_I_EFE(inode) = 1;
 		UDF_UPDATE_UDFREV(inode->i_sb, UDF_VERS_USE_EXTENDED_FE);
 		UDF_I_DATA(inode) = kzalloc(inode->i_sb->s_blocksize - sizeof(struct extendedFileEntry), GFP_KERNEL);
-	}
-	else
-	{
+	} else {
 		UDF_I_EFE(inode) = 0;
 		UDF_I_DATA(inode) = kzalloc(inode->i_sb->s_blocksize - sizeof(struct fileEntry), GFP_KERNEL);
 	}
+	if (!UDF_I_DATA(inode)) {
+		iput(inode);
+		*err = -ENOMEM;
+		mutex_unlock(&sbi->s_alloc_mutex);
+		return NULL;
+	}
 	if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_AD_IN_ICB))
 		UDF_I_ALLOCTYPE(inode) = ICBTAG_FLAG_AD_IN_ICB;
 	else if (UDF_QUERY_FLAG(inode->i_sb, UDF_FLAG_USE_SHORT_AD))

@@ -148,8 +147,7 @@ struct inode * udf_new_inode (struct inode *dir, int mode, int * err)
 	mark_inode_dirty(inode);
 	mutex_unlock(&sbi->s_alloc_mutex);
 
-	if (DQUOT_ALLOC_INODE(inode))
-	{
+	if (DQUOT_ALLOC_INODE(inode)) {
 		DQUOT_DROP(inode);
 		inode->i_flags |= S_NOQUOTA;
 		inode->i_nlink = 0;

index 0842161..ee02bff 100644

index 12613b6..501e54c 100644

index 3b2e6c8..3e937d3 100644

index 67ded28..a6f787f 100644

index 17d3788..c4bd120 100644

Part of the diff is not shown here. View the raw message.

[Jan Kara - Thu, May 24, 2007 at 10:37:14AM +0200]

| On Wed 23-05-07 22:44:36, Cyrill Gorcunov wrote:
| > This patch converts UDF system coding style to kernel coding style.
| > There is no error fixing - just code cleanup. I attempted to make
| > the minimum changes as possible but the patch was growing and growing.
| > So there is a lot of code to review and that could be painful and boring.
| > I've it reviewed several times to assure that the patch does not break
| > current UDF system... but I've only one pair of eyes.
| > 
| > Any comments, suggestions and swearing are welcome. But don't be
| > expected for fast reply - I'm not mature kernel developer ;)
| > 
| > Signed-off-by: Cyrill Gorcunov <[email protected]>
|   Thanks for doing this tedious work... One general comment: Kernel is
| supposed to be viewable on a terminal 80 characters wide so you should
| avoid making longer lines whenever possible (I agree that there are
| cases where wrapped line looks worse that a long line but that's
| just my personal opinion ;).
| 

Jan, I think the problem is about long UDF structures names. The only purpose for this patch is to set up braces and spaces properly. I'll make lines shorter as soon as _this_ patch become veryfied and included in main kernel. Moreover I also should get rid of macroses and other stuff. Lets do work by small steps ;) But you are absolutely right!

| >  static void udf_merge_extents(struct inode *inode,
| > -	 kernel_long_ad laarr[EXTENT_MERGE_SIZE], int *endnum)
| > +			      kernel_long_ad laarr[EXTENT_MERGE_SIZE],
| > +			      int *endnum)
| >  {
| >  	int i;
| >  
| > -	for (i=0; i<(*endnum-1); i++)
| > +	for (i = 0; i < (*endnum - 1); i++)
| >  	{
|   ^^^^^^^ the brace should be on the previous line

Thanks for note. I've it just missed.

| 
| > @@ -1283,86 +1201,74 @@ static void udf_fill_inode(struct inode *inode, struct buffer_head *bh)
| >  		offset = sizeof(struct extendedFileEntry) + UDF_I_LENEATTR(inode);
| >  	}
| >  
| > -	switch (fe->icbTag.fileType)
| > -	{
| > +	switch (fe->icbTag.fileType) {
| >  		case ICBTAG_FILE_TYPE_DIRECTORY:
| > -		{
| > -			inode->i_op = &udf_dir_inode_operations;
| > +			inode->i_op = (struct inode_operations *)&udf_dir_inode_operations;
|   ^^^ Why have you added explicit retyping?

This became from my local git branch. Thanks. Will be removed.

| 
| >  			inode->i_fop = &udf_dir_operations;
| >  			inode->i_mode |= S_IFDIR;
| >  			inc_nlink(inode);
| >  			break;
| > -		}
| >  		case ICBTAG_FILE_TYPE_REALTIME:
| >  		case ICBTAG_FILE_TYPE_REGULAR:
| >  		case ICBTAG_FILE_TYPE_UNDEF:
| > -		{
| >  			if (UDF_I_ALLOCTYPE(inode) == ICBTAG_FLAG_AD_IN_ICB)
| >  				inode->i_data.a_ops = &udf_adinicb_aops;
| >  			else
| >  				inode->i_data.a_ops = &udf_aops;
| > -			inode->i_op = &udf_file_inode_operations;
| > +			inode->i_op = (struct inode_operations *)&udf_file_inode_operations;
|   ^^^ And here again?
| 
| 

Arh!

| > -static mode_t
| > -udf_convert_permissions(struct fileEntry *fe)
| > +static int udf_alloc_i_data(struct inode *inode, size_t size)
| > +{
| > +	UDF_I_DATA(inode) = kmalloc(size, GFP_KERNEL);
| > +
| > +	if (!UDF_I_DATA(inode)) {
| > +		printk(KERN_ERR "udf:udf_alloc_i_data (ino %ld) no free memory\n",
| > +		       inode->i_ino);
| > +		return -ENOMEM;
| > +	}
| > +
| > +	return 0;
| > +}
| > +
|   It seems you've introduced a new function but never used it...

Jan I've messed up with this patch and my prev patches that already in -mm tree. Actually udf_alloc_i_data was introduced by my prev patch. I'll fix this mess.

| 
| 
| > @@ -1573,35 +1465,30 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
| >  	udf_find_anchor(sb);
| >  
| >  	/* Fill in the rest of the superblock */
| > -	sb->s_op = &udf_sb_ops;
| > +	sb->s_op = (struct super_operations *)&udf_sb_ops;
|   Again this explicit typing shouldn't be needed...

Damn, sorry.

| 
| 										Honza
| -- 
| Jan Kara <[email protected]>
| SuSE CR Labs
| 

Andew please drop this patch. I'll make new one (over -mm tree ;)

Cyrill

Hi Cyrill,

but Lindent needs some hand-holding / eyeball help as well...

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

That's true but the point remains: it would be better to have whitespace-only fixes first. But if Andrew is willing to eat Cyrill's patch as-is, I have no real objections either...

[Pekka Enberg - Thu, May 24, 2007 at 09:47:42PM +0300]

| Hi Cyrill,
| 
| On 5/23/07, Cyrill Gorcunov <[email protected]> wrote:
| >@@ -2103,7 +1944,7 @@ long udf_block_map(struct inode *inode, sector_t 
| >block)
| > 	kernel_lb_addr eloc;
| > 	uint32_t elen;
| > 	sector_t offset;
| >-	struct extent_position epos = { NULL, 0, { 0, 0}};
| >+	struct extent_position epos = {};
| 
| Bits like these in the middle of indentation fixes makes me feel
| uneasy. Could we simply ask Andrew to run Lindent on the UDF sources
| at some convinient point and push those to Linus like we did with
| mm/slab.c if I recall correctly? Its safer to do cleanups like these
| after that.
| 

Dunno... Pekka, the patch I sent also added empty lines to separate some code block to be convenient to read. For some parts of code additinal 'if...else..' braces were added. Anyway even the code being formatted by linednt is to be reviewed manually I think. Anyway, your word Andrew?

Cyrill

Such a patch should be whitespace-only: ideally it should generate an identical .o file. There may be legitimate reasons why the .o file isn't identical, but we'd need to understand why that happened.

The way to do this is as two patches (or two-per-C file):

1: feed foo.c through Lindent

2: manual fixups for places where Lindent went wrong.

<fiddles a bit>

OK, so I did

make kernel/sched.s
Lindent kernel/sched.c
make kernel/sched.s

and compared the two sched.s's:

@@ -1713,7 +1713,7 @@
        1:      ud2
 .pushsection __bug_table,"a"
 2:     .long 1b, .LC8  #
-       .word 4306, 0   #
+       .word 4308, 0   #
        .org 2b+12      #
 .popsection
 #NO_APP

@@ -1730,7 +1730,7 @@
        1:      ud2
 .pushsection __bug_table,"a"
 2:     .long 1b, .LC8  #
-       .word 4274, 0   #
+       .word 4276, 0   #
        .org 2b+12      #
 .popsection
 #NO_APP

@@ -2441,7 +2441,7 @@
 .L522:
        movl    $__func__.24446, %eax   #,
        movl    %eax, 12(%esp)  #,
-       movl    $1921, %eax     #,
+       movl    $1920, %eax     #,
        movl    %eax, 8(%esp)   #,
        movl    $.LC8, %eax     #,
        movl    %eax, 4(%esp)   #,

@@ -3855,7 +3855,7 @@
        1:      ud2
 .pushsection __bug_table,"a"
 2:     .long 1b, .LC8  #
-       .word 4078, 0   #
+       .word 4082, 0   #
        .org 2b+12      #
 .popsection
 #NO_APP

those differences are all due to changed line numbers, so we know that this was a safe change.

[Pekka Enberg - Thu, May 24, 2007 at 09:47:42PM +0300]

| Hi Cyrill,
| 
| On 5/23/07, Cyrill Gorcunov <[email protected]> wrote:
| >@@ -2103,7 +1944,7 @@ long udf_block_map(struct inode *inode, sector_t 
| >block)
| > 	kernel_lb_addr eloc;
| > 	uint32_t elen;
| > 	sector_t offset;
| >-	struct extent_position epos = { NULL, 0, { 0, 0}};
| >+	struct extent_position epos = {};
| 
| Bits like these in the middle of indentation fixes makes me feel
| uneasy. Could we simply ask Andrew to run Lindent on the UDF sources
| at some convinient point and push those to Linus like we did with
| mm/slab.c if I recall correctly? Its safer to do cleanups like these
| after that.
| 

And Pekka the patch I sent will be remaked anyway after some fixes. So if you don't like the conversion you pointed - just let me know and I'll not do that ;)

Cyrill

I would prefer whitespace-only first because it's easier to review. But perhaps I am being overly paranoid here. Andrew's call, me thinks.

Anyway, thanks for cleaning the mess up Cyrill!