Re: [f2fs-dev] [PATCH v3] F2FS support

[Jaegeuk Kim]

Thank you for the review.

On Mon, Dec 14, 2015 at 11:28:26AM +0300, Andrei Borzenkov wrote:
> 20.11.2015 00:28, Jaegeuk Kim пишет:
> > Hello,
> > 
> > Change log from v2:
> >  o Enhance the code quality suggested by Andrei
> > 
> > Sorry for the long delay.
> > Could you please check this patch?
> > 
> > Thank you so much,
> > 
> 
> Thank you for continuing to work on it!
> 
> ...
> 
> > +
> > +static inline int
> > +grub_generic_test_bit (int nr, const grub_uint32_t *addr)
> > +{
> > +  return 1UL & (addr[nr / 32] >> (nr & 31));
> > +}
> > +
> 
> As already discussed this code is wrong on big-endian platform. On-disk
> bitmap is little-endian and kernel explicitly uses test_bit_le() here.

Got it.

> 
> > +static inline char *
> > +__inline_addr (struct grub_f2fs_inode *inode)
> > +{
> > +  return (char *)&inode->i_addr[1];
> > +}
> > +
> > +static inline grub_uint64_t
> > +grub_f2fs_file_size (struct grub_f2fs_inode *inode)
> > +{
> > +  return grub_le_to_cpu64 (inode->i_size);
> > +}
> > +
> > +static inline grub_uint32_t
> > +__start_cp_addr (struct grub_f2fs_data *data)
> > +{
> > +  struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> > +  grub_uint64_t ckpt_version = grub_le_to_cpu64 (ckpt->checkpoint_ver);
> > +  grub_uint32_t start_addr = data->cp_blkaddr;
> > +
> > +  if (!(ckpt_version & 1))
> 
> This can use grub_cpu_to_le64_compile_time (1)

Done.

> 
> ...
> 
> > +static inline int
> > +grub_f2fs_test_bit (grub_uint32_t nr, const char *p)
> > +{
> > +  int mask;
> > +
> > +  p += (nr >> 3);
> > +  mask = 1 << (7 - (nr & 0x07));
> > +  return (mask & *p) != 0;
> 
> This is really just "return mask & *p".

Okay.

> 
> > +}
> > +
> > +static int
> > +grub_f2fs_sanity_check_sb (struct grub_f2fs_superblock *sb)
> > +{
> > +  grub_uint32_t log_sectorsize, log_sectors_per_block;
> > +
> > +  if (sb->magic != grub_cpu_to_le32_compile_time (F2FS_SUPER_MAGIC))
> > +    return -1;
> > +
> > +  if (sb->log_blocksize != grub_cpu_to_le32_compile_time (F2FS_BLK_BITS))
> > +    return -1;
> > +
> > +  log_sectorsize = grub_le_to_cpu32 (sb->log_sectorsize);
> > +  log_sectors_per_block = grub_le_to_cpu32 (sb->log_sectors_per_block);
> > +
> > +  if (log_sectorsize > F2FS_MAX_LOG_SECTOR_SIZE)
> > +    return -1;
> > +
> > +  if (log_sectorsize < F2FS_MIN_LOG_SECTOR_SIZE)
> > +    return -1;
> > +
> > +  if (log_sectors_per_block + log_sectorsize != F2FS_MAX_LOG_SECTOR_SIZE)
> 
> This sounds like it should actually be F2FS_BLK_BITS; at least assuming
> that F2FS_MAX_LOG_SECTOR_SIZE may differ from F2FS_BLK_BITS.
> 

Yup. In order to avoid confusion, I removed F2FS_MAX_LOG_SECTOR_SIZE.

> ...
> 
> > +
> > +static grub_ssize_t
> > +grub_f2fs_read_file (grub_fshelp_node_t node,
> > +           grub_disk_read_hook_t read_hook, void *read_hook_data,
> > +           grub_off_t pos, grub_size_t len, char *buf)
> > +{
> > +  struct grub_f2fs_inode *inode = &node->inode.i;
> > +  grub_off_t filesize = grub_f2fs_file_size (inode);
> > +  char *inline_addr = __inline_addr (inode);
> > +
> > +  if (inode->i_inline & F2FS_INLINE_DATA)
> > +    {
> > +      if (pos > filesize || filesize > MAX_INLINE_DATA)
> > +        {
> > +          grub_error (GRUB_ERR_BAD_FS, "corrupted inline_data: need fsck");
> 
> 
> Sorry for confusion, my fault. pos > filesize was OK, but filesize >
> MAX_INLINE_DATA not.

If filesize is over MAX_INLINE_DATA, this file is corrupted as well.
It will cause boundary violation during memcpy below.

> 
> ...
> 
> > +
> > +/* TODO: mkfs.f2fs stores label in a wrong way. Should be fixed. */
> > +static void
> > +grub_f2fs_unicode_to_ascii (grub_uint8_t *out_buf, grub_uint16_t *in_buf)
> > +{
> > +  grub_uint16_t *pchTempPtr = in_buf;
> > +  grub_uint8_t *pwTempPtr = out_buf;
> > +
> > +  while (*pchTempPtr != '\0')
> > +    {
> > +      *pwTempPtr = (grub_uint8_t) *pchTempPtr;
> > +      pchTempPtr++;
> > +      pwTempPtr++;
> > +    }
> > +  *pwTempPtr = '\0';
> > +  return;
> > +}
> 
> Sorry, I do not see how it can work on both big and little endian
> platforms. What byte order is used for on-disk label? Why cannot you use
> grub_utf16_to_utf8 as I asked last time?
> 
> Also please add bundary check, do not rely on correct content.

Got it. I refactored this here and in f2fs-tools as well.
Could you check v4?

Thanks,

> 
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> address@hidden
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel