On Fri, Aug 02, 2013 at 05:49:31PM +0800, Zheng Liu wrote:
> +int ext2fs_process_dir_inline_data(ext2_filsys fs,
> + char *buf,
> + unsigned int buf_len,
> + e2_blkcnt_t blockcnt,
> + struct ext2_inode_large *inode,
> + void *priv_data)
> +{
It looks like there is a lot of code in this function which is in
common with ext2fs_process_dir_block(), so I'd suggest refactoring out
the common code to reduce duplication. This will reduce code size,
and more importantly, improve maintenance of the code.
> +errcode_t ext2fs_inline_data_iterate(ext2_filsys fs,
> + ext2_ino_t ino,
> + int flags,
> + char *block_buf,
> + int (*func)(ext2_filsys fs,
> + char *buf,
> + unsigned int buf_len,
> + e2_blkcnt_t blockcnt,
> + struct ext2_inode_large *inode,
> + void *priv_data),
> + void *priv_data)
This function is misnamed, which worries me a little. First of all,
it only makes sense when called on directories, so some name that
indicates that it is meant to iterate over directories is a good idea.
so some name such as ext2fs_process_inline_data_dir might be a better
choice.
Secondly, it would a really good idea if there was a check to make
sure it was passed an inode number which corresponds to an directory
and that the inline data flag is set. A little paranoia is really
healthy thing --- if we have some application bug where this function
gets called accidentally on an inappropriate inode, we want to return
a clean error code and not stumble on until something bad happens.
> + dirent.inode = (__u32)*inode->i_block;
I'd be much happier with:
dirent.inode = inode->i_block[0];
We shouldn't use casts unless absolutely necessary, and it's not
necessary here.
Also, I suspect we have some byte-swapping problems here. It doesn't
appear there is any allownaces for byte swapping in the inline data
patches. Currently, the ext2fs_read_inode() function will take care
of byte swapping i_blocks[], so that will be OK here, but in the case
of an inode with inline data, if we byte swap all of i_blocks[] then
ext2fs_read_inline_data() will malfunction since the data bytes stored
in the rest of i_blocks[] will be byte swapped. And that would be
wrong.
So I think what you will need to do is to avoid byte swapping the
i_blocks[] array if the inode contains inline_data, and then in the
case where this is a directory, we will need to byte swap i_block[0]
if we are running on a big-endian system.
Cheers,
- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
