Re: [PATCH V3 1/1] fs/udf RFC sanitize types, use consistent naming and add kernel doc comments

[Jan Kara <jack@suse.cz>]

On Sat 14-01-17 11:37:06, Steve Kenton wrote:
> +/*
> + * Unallocated/freespace bitmap implementation routines
> + */
> +
> +/**
> + * udf_bitmap_free_blocks() - Free extent blocks
> + * @sb:		Super block of UDF filesystem mount
> + * @bitmap:	Space bitmap
> + * @eloc:	Extent location/length descriptor

'eloc' is only extent start, not length... It appears several times in the
comments.

> +/*
> + * Unallocated/freespace table implementation routines
> + */
> +
> +/**
> + * udf_table_free_blocks() - Free extent blocks
> + * @sb:		Super block of UDF filesystem mount
> + * @bitmap:	Space table
> + * @eloc:	Extent location/length descriptor
> + * @offset:	Block offset from the start of the extent
> + * @count:	Number of blocks to free
> + *
> + * All the blocks are in the same extent which limits the maximum block count
> + */
>  static void udf_table_free_blocks(struct super_block *sb,
>  				  struct inode *table,
> -				  struct kernel_lb_addr *bloc,
> -				  uint32_t offset,
> -				  uint32_t count)
> +				  struct kernel_lb_addr *eloc,
> +				  sector_t offset,
> +				  int count)
>  {
>  	struct udf_sb_info *sbi = UDF_SB(sb);
>  	struct udf_part_map *partmap;
> -	uint32_t start, end;
> -	uint32_t elen;
> -	struct kernel_lb_addr eloc;
> +	sector_t start, end;
> +	int elen;
> +	struct kernel_lb_addr bloc;
>  	struct extent_position oepos, epos;
> -	int8_t etype;
> +	int etype;
>  	struct udf_inode_info *iinfo;

In this function I find the changes of names confusing. You changed 'bloc'
parameter to 'eloc' which makes sense because it is a beginning of an
extent in which we are freeing. Fine. However then you had to rename local
variable 'eloc' and you renamed it to 'bloc'. This makes is difficult to
catch possible forgotten conversion in this function and also the name
'bloc' does not make sense (it is beginning of an extent in the free table)
and neither it is consistent with matching 'epos', 'etype', and 'elen'
variables. So either leave the argument and local variable names as is, or
rename them so that corresponding variables stay consistenly - something like
'teloc', 'telen', 'tetype', 'tepos'... for 'table extent location'.
  
Otherwise the patch looks good to me. Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR