Re: [PATCH v1 04/22] libext2fs: handle inline data in dir iterator function

[Zheng Liu <gnehzuil.liu@gmail.com>]

On Sun, Oct 13, 2013 at 06:51:12PM -0400, Theodore Ts'o wrote:
> 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.

Yes, Darrick has pointed it out.  I will fix it in next version.

> 
> 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.

Yes, I have noticed that we only byte swap i_block[0], and the following
things don't be swapped.  So I will fix it.

Thanks,
                                                - Zheng