Re: [PATCH]: UDF code style conversion to kernel style

[Cyrill Gorcunov <gorcunov@gmail.com>]

[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 <gorcunov@gmail.com>
|   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 <jack@suse.cz>
| SuSE CR Labs
| 

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

		Cyrill