Re: [PATCH 2/8] Add yaffs2 file system: checkpoint and ecc code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 1 Dec 2010, Charles Manning wrote:

> Signed-off-by: Charles Manning <cdhmanning@xxxxxxxxx>
> ---
>  fs/yaffs2/yaffs_checkptrw.c |  420 +++++++++++++++++++++++++++++++++++++++++++
>  fs/yaffs2/yaffs_checkptrw.h |   33 ++++
>  fs/yaffs2/yaffs_ecc.c       |  298 ++++++++++++++++++++++++++++++
>  fs/yaffs2/yaffs_ecc.h       |   44 +++++
>  4 files changed, 795 insertions(+), 0 deletions(-)
>  create mode 100644 fs/yaffs2/yaffs_checkptrw.c
>  create mode 100644 fs/yaffs2/yaffs_checkptrw.h
>  create mode 100644 fs/yaffs2/yaffs_ecc.c
>  create mode 100644 fs/yaffs2/yaffs_ecc.h
> 

Just a few small comments below.


[...]
> +int yaffs2_get_checkpt_sum(struct yaffs_dev *dev, u32 * sum)
> +{
> +	u32 composite_sum;
> +	composite_sum = (dev->checkpt_sum << 8) | (dev->checkpt_xor & 0xFF);
> +	*sum = composite_sum;
> +	return 1;
> +}

Why not get rid of the redundant local variable and simply do

  *sum = (dev->checkpt_sum << 8) | (dev->checkpt_xor & 0xFF);

??


[...]
> +int yaffs2_checkpt_wr(struct yaffs_dev *dev, const void *data, int n_bytes)
> +{
> +	int i = 0;
> +	int ok = 1;
> +
> +	u8 *data_bytes = (u8 *) data;

'data' is a 'const void *' and this casts away const. Probably fine, just 
makes my toes curl.


[...]
> +int yaffs2_checkpt_rd(struct yaffs_dev *dev, void *data, int n_bytes)
> +{
> +	int i = 0;
> +	int ok = 1;
> +	struct yaffs_ext_tags tags;
> +
> +	int chunk;
> +	int realigned_chunk;
> +
> +	u8 *data_bytes = (u8 *) data;

here you are not casting away const, but since 'data' is a void pointer 
the cast is just completely superfluous and should just go away.


[...]
> +			if (dev->checkpt_cur_block < 0)
> +				ok = 0;
> +			else {

Nitpicking, but this should be 

  if (dev->checkpt_cur_block < 0) {
    ok = 0;
  } else {
  ...

curly brace on both branches since it's used on one of them (according to 
CodingStyle).


[...]
> +int yaffs_checkpt_close(struct yaffs_dev *dev)
> +{
> +
> +	if (dev->checkpt_open_write) {
> +		if (dev->checkpt_byte_offs != 0)
> +			yaffs2_checkpt_flush_buffer(dev);
> +	} else if (dev->checkpt_block_list) {
> +		int i;
> +		for (i = 0;
> +		     i < dev->blocks_in_checkpt
> +		     && dev->checkpt_block_list[i] >= 0; i++) {
> +			int blk = dev->checkpt_block_list[i];
> +			struct yaffs_block_info *bi = NULL;
> +			if (dev->internal_start_block <= blk
> +			    && blk <= dev->internal_end_block)
> +				bi = yaffs_get_block_info(dev, blk);
> +			if (bi && bi->block_state == YAFFS_BLOCK_STATE_EMPTY)
> +				bi->block_state = YAFFS_BLOCK_STATE_CHECKPOINT;
> +			else {
> +				/* Todo this looks odd... */
> +			}

1) nitpicking about curly braces again (see above).
2) try grep'ing the source for "todo", "xxx", "fixme" and similar, then 
check the age of those comments. If things like this are not addressed (by 
other than a "todo" comment) on initial commit they stand a good chance of 
never getting addressed...


-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux