Re: [PATCH v3] F2FS support

[Andrei Borzenkov]

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.

> +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)

...

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

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

...

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

...

> +
> +/* 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.