Hi,
On Fri, 04 Dec 2009 18:23:30 +0900, Jiro SEKIBA <j...@unicus.jp> wrote:
> Hi,
>
> This is a trivial style fix patch to mend errors/warnings
> reported by "checkpatch.pl --file".
>
> Signed-off-by: Jiro SEKIBA <j...@unicus.jp>
> ---
> fs/nilfs2/bmap.c | 3 ++-
> fs/nilfs2/cpfile.c | 5 +++--
> fs/nilfs2/direct.c | 14 ++++++++------
> 3 files changed, 13 insertions(+), 9 deletions(-)
Ok, I'll comment inline below.
> diff --git a/fs/nilfs2/bmap.c b/fs/nilfs2/bmap.c
> index 08834df..78ccbba 100644
> --- a/fs/nilfs2/bmap.c
> +++ b/fs/nilfs2/bmap.c
> @@ -426,7 +426,8 @@ __u64 nilfs_bmap_data_get_key(const struct nilfs_bmap
> *bmap,
> key = page_index(bh->b_page) << (PAGE_CACHE_SHIFT -
> bmap->b_inode->i_blkbits);
> for (pbh = page_buffers(bh->b_page); pbh != bh;
> - pbh = pbh->b_this_page, key++);
> + pbh = pbh->b_this_page, key++)
> + ;
How about moving the "key++" inside the loop? The line break in the
middle of the for-sentence looks eliminable.
> return key;
> }
> diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
> index 3f5d5d0..a86d402 100644
> --- a/fs/nilfs2/cpfile.c
> +++ b/fs/nilfs2/cpfile.c
> @@ -328,9 +328,10 @@ int nilfs_cpfile_delete_checkpoints(struct inode *cpfile,
> tnicps += nicps;
> nilfs_mdt_mark_buffer_dirty(cp_bh);
> nilfs_mdt_mark_dirty(cpfile);
> + count = nilfs_cpfile_block_sub_valid_checkpoints(
> + cpfile, cp_bh, kaddr, nicps);
> if (!nilfs_cpfile_is_in_first(cpfile, cno) &&
> - (count = nilfs_cpfile_block_sub_valid_checkpoints(
> - cpfile, cp_bh, kaddr, nicps)) == 0) {
> + count == 0) {
This conversion is not equivalent because
nilfs_cpfile_block_sub_valid_checkpoints() has a side effect.
> /* make hole */
> kunmap_atomic(kaddr, KM_USER0);
> brelse(cp_bh);
> diff --git a/fs/nilfs2/direct.c b/fs/nilfs2/direct.c
> index d369ac7..f817282 100644
> --- a/fs/nilfs2/direct.c
> +++ b/fs/nilfs2/direct.c
> @@ -53,9 +53,10 @@ static int nilfs_direct_lookup(const struct nilfs_bmap
> *bmap,
>
> direct = (struct nilfs_direct *)bmap;
> if ((key > NILFS_DIRECT_KEY_MAX) ||
> - (level != 1) || /* XXX: use macro for level 1 */
> - ((ptr = nilfs_direct_get_ptr(direct, key)) ==
> - NILFS_BMAP_INVALID_PTR))
> + (level != 1)) /* XXX: use macro for level 1 */
> + return -ENOENT;
> + ptr = nilfs_direct_get_ptr(direct, key);
> + if (ptr == NILFS_BMAP_INVALID_PTR)
> return -ENOENT;
Looks good, but it's still redundant. How about removing unnecessary
parentheses in this occasion?
The first conditional statement can be simplified to:
if (key > NILFS_DIRECT_KEY_MAX || level != 1)
> if (ptrp != NULL)
> @@ -73,9 +74,10 @@ static int nilfs_direct_lookup_contig(const struct
> nilfs_bmap *bmap,
> sector_t blocknr;
> int ret, cnt;
>
> - if (key > NILFS_DIRECT_KEY_MAX ||
> - (ptr = nilfs_direct_get_ptr(direct, key)) ==
> - NILFS_BMAP_INVALID_PTR)
> + if (key > NILFS_DIRECT_KEY_MAX)
> + return -ENOENT;
> + ptr = nilfs_direct_get_ptr(direct, key);
> + if (ptr == NILFS_BMAP_INVALID_PTR)
> return -ENOENT;
Looks good to me.
> if (NILFS_BMAP_USE_VBN(bmap)) {
> --
> 1.5.6.5
Thanks,
Ryusuke Konishi
_______________________________________________
users mailing list
users@nilfs.org
https://www.nilfs.org/mailman/listinfo/users
