Re: [PATCH] F2FS support

[Jaegeuk Kim <jaegeuk@kernel.org>]

Hi Andrei,

On Sat, Mar 28, 2015 at 10:31:55AM +0300, Andrei Borzenkov wrote:
> В Tue, 24 Mar 2015 01:19:00 -0700
> Jaegeuk Kim <jaegeuk@kernel.org> пишет:
> 
> >  * Makefile.util.def: Add f2fs.c.
> >  * doc/grub.texi: Add f2fs description.
> >  * grub-core/Makefile.core.def: Add f2fs module.
> >  * grub-core/fs/f2fs.c: New file.
> >  * tests/f2fs_test.in: New file.
> >  * tests/util/grub-fs-tester.in: Add f2fs requirements.
> > 
> 
> It's not the most useful commit message. Better would be short
> explanation of use cases and intended platforms. I'm curious here -
> F2FS is intended for raw flash access, on which platform(s) grub has
> access to such devices? 

I just followed the commit convention in grub.git.
Anyway I'll add some description in v2.

F2FS is *not* intended for raw flash, for general block device such as SSD,
eMMC, and SD cards.
Please refer the following documents.

http://en.wikipedia.org/wiki/F2FS

Thank you for the detailed review.
I'll fix them and send v2.

Sincerely yours,

> 
> > 
> > diff --git a/ChangeLog-2015 b/ChangeLog-2015
> 
> We do not use ChangeLog any more, it is autogenerated from commits.
> This file is legacy before this change, do not change it.
> 
> > diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c
> > new file mode 100644
> > index 0000000..40360d5
> > --- /dev/null
> > +++ b/grub-core/fs/f2fs.c
> > @@ -0,0 +1,1321 @@
> > +/*
> > + *  f2fs.c - Flash-Friendly File System
> > + *
> > + *  Written by Jaegeuk Kim <jaegeuk@kernel.org>
> > + *
> > + *  Copyright (C) 2015  Free Software Foundation, Inc.
> > + *
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +#include <grub/err.h>
> > +#include <grub/file.h>
> > +#include <grub/mm.h>
> > +#include <grub/misc.h>
> > +#include <grub/disk.h>
> > +#include <grub/dl.h>
> > +#include <grub/types.h>
> > +#include <grub/charset.h>
> > +#include <grub/fshelp.h>
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +
> > +/* F2FS Magic Number */
> > +#define F2FS_SUPER_MAGIC	0xF2F52010
> > +
> > +/* byte-size offset */
> > +#define F2FS_SUPER_OFFSET		1024
> > +
> > +/* 12 bits for 4096 bytes */
> > +#define F2FS_MAX_LOG_SECTOR_SIZE	12
> > +
> > +/* 9 bits for 512 bytes */
> > +#define F2FS_MIN_LOG_SECTOR_SIZE	9
> > +
> > +/* support only 4KB block */
> > +#define F2FS_BLKSIZE			4096
> 
> (2 << F2FS_BLK_BITS)?
> 
> > +#define F2FS_BLK_BITS			12
> > +#define F2FS_BLK_SEC_BITS		(3)
> 
> 
> It is confusing to have some defines parenthesized and some not. Could
> it be unified somehow?
> 
> Also this can be computed from F2FS_BLK_BITS and GRUB_DISK_SECTOR_BITS
> - one magic number less.
> 
> ...
> > +struct grub_f2fs_inline_dentry
> > +{
> > +  grub_uint8_t dentry_bitmap[INLINE_DENTRY_BITMAP_SIZE];
> 
> This is cast to grub_uint32_t everywhere. Can it be non-multiple of 4
> bytes? If not, may be just define as such? 
> 
> > +  grub_uint8_t reserved[INLINE_RESERVED_SIZE];
> > +  struct grub_f2fs_dir_entry dentry[NR_INLINE_DENTRY];
> > +  grub_uint8_t filename[NR_INLINE_DENTRY][F2FS_SLOT_LEN];
> > +} GRUB_PACKED;
> > +
> > +struct grub_f2fs_dentry_block {
> > +  grub_uint8_t dentry_bitmap[SIZE_OF_DENTRY_BITMAP];
> 
> ditto
> 
> > +  grub_uint8_t reserved[SIZE_OF_RESERVED];
> > +  struct grub_f2fs_dir_entry dentry[NR_DENTRY_IN_BLOCK];
> > +  grub_uint8_t filename[NR_DENTRY_IN_BLOCK][F2FS_SLOT_LEN];
> > +} GRUB_PACKED;
> > +
> 
> 
> ...
> > +
> > +#define ver_after (a, b) (typecheck (unsigned long long, a) &&          \
> > +		typecheck (unsigned long long, b) &&                    \
> > +		((long long)((a) - (b)) > 0))
> > +
> 
> Where typecheck definition comes from?
> 
> ...
> > +
> > +static inline int
> > +__test_bit (int nr, grub_uint32_t *addr)
> > +{
> > +  return 1UL & (addr[nr / 32] >> (nr & (31)));
> Extra parenthesis (31)
> 
> > +}
> > +
> 
> It is used for dentry_bitmap which is kept in LE format and not
> converted as far as I can tell. This needs fixing for BE systems. Linux
> kernel is explicitly using test_bit_le here. This will also work for
> inode flags (just skip explicit conversion).
> 
> There are two functions with more or less identical names. May be make
> them
> 
> grub_f2fs_test_bit_le32
> grub_f2fs_test_bit
> 
> As a general comment - marking non-modified arguments as const
> everywhere would be good.
> 
> > +static inline char *
> > +__inline_addr (struct grub_f2fs_inode *inode)
> > +{
> > +  return (char *)&(inode->i_addr[1]);
> Redundant parens around inode->
> 
> > +}
> > +
> > +static inline grub_uint64_t
> > +__i_size (struct grub_f2fs_inode *inode)
> 
> Could we make it grub_f2fs_file_size or similar? i_size really does not
> tell much outside of linux kernel.
> 
> > +{
> > +  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))
> > +    return start_addr + data->blocks_per_seg;
> > +  return start_addr;
> > +}
> > +
> > +static inline grub_uint32_t
> > +__start_sum_block (struct grub_f2fs_data *data)
> > +{
> > +  struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> > +
> > +  return __start_cp_addr (data) + grub_le_to_cpu32 (ckpt->cp_pack_start_sum);
> > +}
> > +
> > +static inline grub_uint32_t
> > +__sum_blk_addr (struct grub_f2fs_data *data, int base, int type)
> > +{
> > +  struct grub_f2fs_checkpoint *ckpt = &data->ckpt;
> > +
> > +  return __start_cp_addr (data) +
> > +		grub_le_to_cpu32 (ckpt->cp_pack_total_block_count)
> > +		- (base + 1) + type;
> > +}
> > +
> > +static inline int
> > +__ckpt_flag_set (struct grub_f2fs_checkpoint *ckpt, unsigned int f)
> > +{
> > +  grub_uint32_t ckpt_flags = grub_le_to_cpu32 (ckpt->ckpt_flags);
> > +  return ckpt_flags & f;
> 
> All flags are constant so you can simply do 
> 
> ckpt->ckpt_flags & grub_cpu_to_le32_compile_time (FLAG)
> 
> in place to avoid extra calls. This makes function redundant.
> 
> > +}
> > +
> > +static inline int
> > +__inode_flag_set (struct grub_f2fs_inode *inode, int flag)
> > +{
> > +  grub_uint32_t i_flags = grub_le_to_cpu32 (inode->i_flags);
> > +  return __test_bit (flag, &i_flags);
> > +}
> 
> grub_f2fs_test_bit_le32?
> 
> > +
> > +/*
> > + * CRC32
> > + */
> > +#define CRCPOLY_LE 0xedb88320
> > +
> > +static inline grub_uint32_t
> > +grub_f2fs_cal_crc32 (grub_uint32_t crc, void *buf, int len)
> 
> Why crc is parameter here? This function is used exactly once with
> fixed value for initial crc.
> 
> > +{
> > +  int i;
> > +  unsigned char *p = (unsigned char *)buf;
> > +
> > +  while (len--)
> > +    {
> > +      crc ^= *p++;
> > +      for (i = 0; i < 8; i++)
> > +        crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY_LE : 0);
> > +    }
> > +  return crc;
> > +}
> > +
> > +static inline int
> > +grub_f2fs_crc_valid (grub_uint32_t blk_crc, void *buf, int len)
> > +{
> > +  grub_uint32_t cal_crc = 0;
> > +
> > +  cal_crc = grub_f2fs_cal_crc32 (F2FS_SUPER_MAGIC, buf, len);
> > +
> > +  return (cal_crc == blk_crc) ? 1 : 0;
> > +}
> > +
> > +static inline int
> > +grub_f2fs_test_bit (grub_uint32_t nr, const char *p)
> > +{
> > +  int mask;
> > +  char *addr = (char *)p;
> 
> Why cast? We are not going to modify it, right?
> 
> > +
> > +  addr += (nr >> 3);
> > +  mask = 1 << (7 - (nr & 0x07));
> > +  return (mask & *addr) != 0;
> > +}
> > +
> > +static int
> > +grub_f2fs_sanity_check_sb (struct grub_f2fs_superblock *sb)
> > +{
> > +  unsigned int blocksize;
> > +
> > +  if (F2FS_SUPER_MAGIC != grub_le_to_cpu32 (sb->magic))
> 
> sb->magic != grub_cpu_to_le32_compile_time (F2FS_SUPER_MAGIC)
> 
> > +    return -1;
> > +
> > +  blocksize = 1 << grub_le_to_cpu32 (sb->log_blocksize);
> > +  if (blocksize != F2FS_BLKSIZE)
> 
> sb->log_blksize != grub_cpu_to_le32_compile_time (F2FS_BLK_BITS)
> 
> > +    return -1;
> > +
> > +  if (grub_le_to_cpu32 (sb->log_sectorsize) > F2FS_MAX_LOG_SECTOR_SIZE)
> > +    return -1;
> > +
> > +  if (grub_le_to_cpu32 (sb->log_sectorsize) < F2FS_MIN_LOG_SECTOR_SIZE)
> > +    return -1;
> > +
> > +  if (grub_le_to_cpu32 (sb->log_sectors_per_block) +
> > +      grub_le_to_cpu32 (sb->log_sectorsize) != F2FS_MAX_LOG_SECTOR_SIZE)
> 
> Should not it be F2FS_BLKSIZE? At least it sounds logical. Also please
> convert log_sectorsize just once.
> 
> > +    return -1;
> > +
> > +  return 0;
> > +}
> > +
> > +static grub_err_t
> > +grub_f2fs_read_sb (struct grub_f2fs_data *data, int block)
> > +{
> > +  grub_disk_t disk = data->disk;
> > +  grub_uint64_t offset;
> > +  grub_err_t err;
> > +
> > +  if (block == 0)
> > +    offset = F2FS_SUPER_OFFSET;
> > +  else
> > +    offset = F2FS_BLKSIZE + F2FS_SUPER_OFFSET;
> > +
> 
> Please name it "secondary" or similar instead of "block" to avoid
> confusion. You do not really want to read arbitrary block, right?
> 
> offset = F2FS_SUPER_OFFEST;
> if (secondary)
>   offset += F2FS_BLKSIZE;
> 
> > +  /* Read first super block. */
> > +  err = grub_disk_read (disk, offset >> GRUB_DISK_SECTOR_BITS, 0,
> > +			sizeof (data->sblock), &data->sblock);
> > +  if (err)
> > +    return err;
> > +
> > +  if (grub_f2fs_sanity_check_sb (&data->sblock))
> > +    err = GRUB_ERR_BAD_FS;
> > +
> > +  return err;
> > +}
> > +
> > +static void *
> > +validate_checkpoint (struct grub_f2fs_data *data, grub_uint32_t cp_addr,
> > +						grub_uint64_t *version)
> > +{
> > +  void *cp_page_1, *cp_page_2;
> > +  struct grub_f2fs_checkpoint *cp_block;
> > +  grub_uint64_t cur_version = 0, pre_version = 0;
> > +  grub_uint32_t crc = 0;
> > +  grub_uint32_t crc_offset;
> > +  grub_err_t err;
> > +
> > +  /* Read the 1st cp block in this CP pack */
> > +  cp_page_1 = grub_malloc (F2FS_BLKSIZE);
> > +  if (!cp_page_1)
> > +    return NULL;
> > +
> > +  err = grub_f2fs_block_read (data, cp_addr, cp_page_1);
> > +  if (err)
> > +    goto invalid_cp1;
> > +
> > +  cp_block = (struct grub_f2fs_checkpoint *)cp_page_1;
> > +  crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> > +  if (crc_offset >= F2FS_BLKSIZE)
> > +    goto invalid_cp1;
> > +
> > +  crc = *(grub_uint32_t *)((char *)cp_block + crc_offset);
> 
> Is unaligned access possible here? If yes, it probably should be
> grub_get_unaligned32.
> 
> > +  if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> > +    goto invalid_cp1;
> > +
> 
> Should not CRC be converted from LE?
> 
> > +  pre_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> > +
> > +  /* Read the 2nd cp block in this CP pack */
> > +  cp_page_2 = grub_malloc (F2FS_BLKSIZE);
> > +  if (!cp_page_2)
> > +    goto invalid_cp1;
> > +
> > +  cp_addr += grub_le_to_cpu32 (cp_block->cp_pack_total_block_count) - 1;
> > +
> > +  err = grub_f2fs_block_read (data, cp_addr, cp_page_2);
> > +  if (err)
> > +    goto invalid_cp2;
> > +
> > +  cp_block = (struct grub_f2fs_checkpoint *)cp_page_2;
> > +  crc_offset = grub_le_to_cpu32 (cp_block->checksum_offset);
> > +  if (crc_offset >= F2FS_BLKSIZE)
> > +    goto invalid_cp2;
> > +
> > +  crc = *(grub_uint32_t *)((char *)cp_block + crc_offset);
> 
> Ditto alignment.
> 
> > +  if (!grub_f2fs_crc_valid (crc, cp_block, crc_offset))
> Ditto endianness.
> 
> > +    goto invalid_cp2;
> > +
> > +  cur_version = grub_le_to_cpu64 (cp_block->checkpoint_ver);
> > +  if (cur_version == pre_version)
> > +    {
> > +      *version = cur_version;
> > +      grub_free (cp_page_2);
> > +      return cp_page_1;
> > +    }
> > +
> > +invalid_cp2:
> > +  grub_free (cp_page_2);
> > +invalid_cp1:
> > +  grub_free (cp_page_1);
> > +  return NULL;
> > +}
> > +
> > +static grub_err_t
> > +grub_f2fs_read_cp (struct grub_f2fs_data *data)
> > +{
> > +  void *cp1, *cp2, *cur_page;
> > +  grub_uint64_t cp1_version = 0, cp2_version = 0;
> > +  grub_uint64_t cp_start_blk_no;
> > +
> > +  /*
> > +   * Finding out valid cp block involves read both
> > +   * sets (cp pack1 and cp pack 2)
> > +   */
> > +  cp_start_blk_no = data->cp_blkaddr;
> > +  cp1 = validate_checkpoint (data, cp_start_blk_no, &cp1_version);
> > +  if (!cp1 && grub_errno)
> > +      return grub_errno;
> > +
> > +  /* The second checkpoint pack should start at the next segment */
> > +  cp_start_blk_no += data->blocks_per_seg;
> > +  cp2 = validate_checkpoint (data, cp_start_blk_no, &cp2_version);
> > +  if (!cp2 && grub_errno)
> > +    {
> > +      grub_free (cp1);
> > +      return grub_errno;
> > +    }
> > +
> > +  if (cp1 && cp2)
> > +    cur_page = (cp2_version > cp1_version) ? cp2 : cp1;
> > +  else if (cp1)
> > +    cur_page = cp1;
> > +  else if (cp2)
> > +    cur_page = cp2;
> > +  else
> > +    return grub_error (GRUB_ERR_BAD_FS, "no checkpoints\n");
> > +
> > +  grub_memcpy (&data->ckpt, cur_page, F2FS_BLKSIZE);
> > +
> > +  grub_free (cp1);
> > +  grub_free (cp2);
> > +  return 0;
> > +}
> > +
> > +static int
> 
> static grub_error_t
> 
> > +get_nat_journal (struct grub_f2fs_data *data)
> > +{
> > +  grub_uint32_t block;
> > +  char *buf;
> > +  grub_err_t err;
> > +
> > +  buf = grub_malloc (F2FS_BLKSIZE);
> > +  if (!buf)
> > +    return grub_errno;
> > +
> > +  if (__ckpt_flag_set (&data->ckpt, CP_COMPACT_SUM_FLAG))
> > +    block = __start_sum_block (data);
> > +  else if (__ckpt_flag_set (&data->ckpt, CP_UMOUNT_FLAG))
> 
> As mentioned, use grub_cpu_to_leXX_compile_time to avoid run time
> conversion.
> 
> > +    block = __sum_blk_addr (data, NR_CURSEG_TYPE, CURSEG_HOT_DATA);
> > +  else
> > +    block = __sum_blk_addr (data, NR_CURSEG_DATA_TYPE, CURSEG_HOT_DATA);
> > +
> > +  err = grub_f2fs_block_read (data, block, buf);
> > +  if (err)
> > +    goto fail;
> > +
> > +  if (__ckpt_flag_set (&data->ckpt, CP_COMPACT_SUM_FLAG))
> > +    grub_memcpy (&data->nat_j, buf, SUM_JOURNAL_SIZE);
> > +  else
> > +    grub_memcpy (&data->nat_j, buf + SUM_ENTRIES_SIZE, SUM_JOURNAL_SIZE);
> > +
> > +fail:
> > +  grub_free (buf);
> > +  return err;
> > +}
> > +
> ...
> > +static int
> > +grub_get_node_path (struct grub_f2fs_inode *inode, grub_uint32_t block,
> > +			grub_uint32_t offset[4], grub_uint32_t noffset[4])
> > +{
> > +  grub_uint32_t direct_blks = ADDRS_PER_BLOCK;
> > +  grub_uint32_t dptrs_per_blk = NIDS_PER_BLOCK;
> > +  grub_uint32_t indirect_blks = ADDRS_PER_BLOCK * NIDS_PER_BLOCK;
> > +  grub_uint32_t dindirect_blks = indirect_blks * NIDS_PER_BLOCK;
> > +  grub_uint32_t direct_index = DEF_ADDRS_PER_INODE;
> > +  int n = 0;
> > +  int level = 0;
> > +
> > +  if (__inode_flag_set (inode, FI_INLINE_XATTR))
> > +    direct_index -= F2FS_INLINE_XATTR_ADDRS;
> > +
> > +  noffset[0] = 0;
> > +
> > +  if (block < direct_index)
> > +    {
> > +      offset[n] = block;
> > +      goto got;
> > +    }
> > +
> > +  block -= direct_index;
> > +  if (block < direct_blks)
> > +    {
> > +      offset[n++] = NODE_DIR1_BLOCK;
> > +      noffset[n] = 1;
> > +      offset[n] = block;
> > +      level = 1;
> > +      goto got;
> > +    }
> > +
> > +  block -= direct_blks;
> > +  if (block < direct_blks)
> > +    {
> > +      offset[n++] = NODE_DIR2_BLOCK;
> > +      noffset[n] = 2;
> > +      offset[n] = block;
> > +      level = 1;
> > +      goto got;
> > +    }
> > +
> > +  block -= direct_blks;
> > +  if (block < indirect_blks)
> > +    {
> > +      offset[n++] = NODE_IND1_BLOCK;
> > +      noffset[n] = 3;
> > +      offset[n++] = block / direct_blks;
> > +      noffset[n] = 4 + offset[n - 1];
> 
> That does not fit. You declared offset and noffset as arrays of four
> elements and pass arrays of four elements; here is out of bound
> access already.
> 
> > +      offset[n] = block % direct_blks;
> > +      level = 2;
> > +      goto got;
> > +    }
> > +
> > +  block -= indirect_blks;
> > +  if (block < indirect_blks)
> > +    {
> > +      offset[n++] = NODE_IND2_BLOCK;
> > +      noffset[n] = 4 + dptrs_per_blk;
> > +      offset[n++] = block / direct_blks;
> > +      noffset[n] = 5 + dptrs_per_blk + offset[n - 1];
> > +      offset[n] = block % direct_blks;
> > +      level = 2;
> > +      goto got;
> > +    }
> > +
> > +  block -= indirect_blks;
> > +  if (block < dindirect_blks)
> > +    {
> > +      offset[n++] = NODE_DIND_BLOCK;
> > +      noffset[n] = 5 + (dptrs_per_blk * 2);
> > +      offset[n++] = block / indirect_blks;
> > +      noffset[n] = 6 + (dptrs_per_blk * 2) +
> > +		offset[n - 1] * (dptrs_per_blk + 1);
> > +      offset[n++] = (block / direct_blks) % dptrs_per_blk;
> > +      noffset[n] = 7 + (dptrs_per_blk * 2) +
> > +		offset[n - 2] * (dptrs_per_blk + 1) +
> > +		offset[n - 1];
> > +      offset[n] = block % direct_blks;
> > +      level = 3;
> > +      goto got;
> > +    }
> > +got:
> > +  return level;
> > +}
> > +
> > +
> > +static grub_err_t
> > +load_nat_info (struct grub_f2fs_data *data)
> > +{
> > +  void *version_bitmap;
> > +  grub_err_t err;
> > +
> > +  data->nat_bitmap = grub_malloc (__nat_bitmap_size (data));
> > +  if (!data->nat_bitmap)
> > +    return grub_errno;
> > +
> > +  version_bitmap = __nat_bitmap_ptr (data);
> > +
> > +  /* copy version bitmap */
> > +  grub_memcpy (data->nat_bitmap, version_bitmap, __nat_bitmap_size (data));
> > +
> 
> Any reason to actually copy it? Why is it not possible to just set
> pointer to source, which is available all the time anyway?
> 
> > +  err = get_nat_journal (data);
> > +  if (err)
> > +    grub_free (data->nat_bitmap);
> > +
> > +  return err;
> > +}
> > +
> > +static grub_err_t
> > +grub_f2fs_read_node (struct grub_f2fs_data *data,
> > +			grub_uint32_t nid, struct grub_f2fs_node *np)
> > +{
> > +  grub_uint32_t blkaddr;
> > +
> > +  blkaddr = get_node_blkaddr (data, nid);
> > +  if (!blkaddr)
> > +    return grub_errno;
> > +
> > +  return grub_f2fs_block_read (data, blkaddr, np);
> 
> Is struct grub_f2fs_node guaranteed to always have the same size as F2FS
> block? Then adding char [F2FS_BLKSIZE] to union to make it obvious is
> better and ensures that it will always be at least this size.
> 
> > +}
> > +
> > +static struct grub_f2fs_data *
> > +grub_f2fs_mount (grub_disk_t disk)
> > +{
> > +  struct grub_f2fs_data *data;
> > +  grub_err_t err;
> > +
> > +  data = grub_zalloc (sizeof (*data));
> > +  if (!data)
> > +    return NULL;
> > +
> > +  data->disk = disk;
> > +
> > +  err = grub_f2fs_read_sb (data, 0);
> > +  if (err)
> > +    {
> > +      err = grub_f2fs_read_sb (data, 1);
> > +      if (err)
> > +        {
> > +          grub_error (GRUB_ERR_BAD_FS, "not a F2FS filesystem");
> 
> May be mentioning that superblock could not be read? In another place
> you already tell that checkpoints could not be found. It helps to
> troubleshoot issues.
> 
> > +          goto fail;
> > +	}
> > +    }
> > +
> > +  data->root_ino = grub_le_to_cpu32 (data->sblock.root_ino);
> > +  data->cp_blkaddr = grub_le_to_cpu32 (data->sblock.cp_blkaddr);
> > +  data->nat_blkaddr = grub_le_to_cpu32 (data->sblock.nat_blkaddr);
> > +  data->blocks_per_seg = 1 <<
> > +		grub_le_to_cpu32 (data->sblock.log_blocks_per_seg);
> > +
> > +  err = grub_f2fs_read_cp (data);
> > +  if (err)
> > +    goto fail;
> > +
> > +  err = load_nat_info (data);
> > +  if (err)
> > +    goto fail;
> > +
> > +  data->diropen.data = data;
> > +  data->diropen.ino = data->root_ino;
> > +  data->diropen.inode_read = 1;
> > +  data->inode = &data->diropen.inode;
> > +
> > +  err = grub_f2fs_read_node (data, data->root_ino, data->inode);
> > +  if (err)
> > +    goto fail;
> > +
> > +  return data;
> > +
> > +fail:
> > +  if (data)
> > +    grub_free (data->nat_bitmap);
> 
> Double free after load_nat_info failure. Assuming that we do need to
> allocate anything at all (see above).
> 
> > +  grub_free (data);
> > +  return NULL;
> > +}
> > +
> > +/* guarantee inline_data was handled by caller */
> > +static grub_disk_addr_t
> > +grub_f2fs_read_block (grub_fshelp_node_t node, grub_disk_addr_t block_ofs)
> 
> You have grub_f2fs_read_block and grub_f2fs_block_read. Could we make
> them more different and self-explaining? In particular, this one does
> not read anything, it returns disk address. grub_f2fs_map_file_block?
> 
> > +{
> > +  struct grub_f2fs_data *data = node->data;
> > +  struct grub_f2fs_inode *inode = &node->inode.i;
> > +  grub_uint32_t offset[4], noffset[4], nids[4];
> 
> See above about overflow in grub_get_inode_path.
> 
> > +  struct grub_f2fs_node *node_block;
> > +  grub_uint32_t block_addr = -1;
> > +  int level, i;
> > +
> > +  level = grub_get_node_path (inode, block_ofs, offset, noffset);
> > +  if (level == 0)
> > +      return grub_le_to_cpu32 (inode->i_addr[offset[0]]);
> > +
> > +  node_block = grub_malloc (F2FS_BLKSIZE);
> > +  if (!node_block)
> > +    return -1;
> > +
> > +  nids[1] = __get_node_id (&node->inode, offset[0], 1);
> > +
> > +  /* get indirect or direct nodes */
> > +  for (i = 1; i <= level; i++)
> > +    {
> > +      grub_f2fs_read_node (data, nids[i], node_block);
> > +      if (grub_errno)
> > +        goto fail;
> > +
> > +      if (i < level)
> > +        nids[i + 1] = __get_node_id (node_block, offset[i], 0);
> > +    }
> > +
> > +  block_addr = grub_le_to_cpu32 (node_block->dn.addr[offset[level]]);
> > +fail:
> > +  grub_free (node_block);
> > +  return block_addr;
> > +}
> > +
> ...
> > +
> > +static char *
> > +grub_f2fs_read_symlink (grub_fshelp_node_t node)
> > +{
> > +  char *symlink;
> > +  struct grub_fshelp_node *diro = node;
> > +
> > +  if (!diro->inode_read)
> > +    {
> > +      grub_f2fs_read_node (diro->data, diro->ino, &diro->inode);
> > +      if (grub_errno)
> > +	return 0;
> > +    }
> > +
> > +  symlink = grub_malloc (__i_size (&diro->inode.i) + 1);
> > +  if (!symlink)
> > +    return 0;
> > +
> > +  grub_f2fs_read_file (diro, 0, 0, 0, __i_size (&diro->inode.i), symlink);
> > +  if (grub_errno)
> > +    {
> > +      grub_free (symlink);
> > +      return 0;
> > +    }
> > +
> 
> What about short read? Is this an error or not?
> 
> > +  symlink[__i_size (&diro->inode.i)] = '\0';
> > +  return symlink;
> > +}
> > +
> > +static int
> > +grub_f2fs_check_dentries (struct grub_f2fs_dir_iter_ctx *ctx)
> > +{
> > +  struct grub_fshelp_node *fdiro;
> > +  int i;
> > +
> > +  for (i = 0; i < ctx->max;)
> > +    {
> > +      char filename[F2FS_NAME_LEN + 1];
> 
> Could we avoid large stack allocations?
> 
> > +      enum grub_fshelp_filetype type = GRUB_FSHELP_UNKNOWN;
> > +      enum FILE_TYPE ftype;
> > +      int name_len;
> > +
> > +      if (__test_bit (i, ctx->bitmap) == 0)
> 
> grub_f2fs_test_bit_le32?
> 
> > +        {
> > +          i++;
> > +          continue;
> > +        }
> > +
> > +      ftype = ctx->dentry[i].file_type;
> > +      name_len = grub_le_to_cpu16 (ctx->dentry[i].name_len);
> > +      grub_memcpy (filename, ctx->filename[i], name_len);
> > +      filename[name_len] = '\0';
> > +
> > +      fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
> > +      if (!fdiro)
> > +        return 0;
> > +
> > +      if (ftype == F2FS_FT_DIR)
> > +        type = GRUB_FSHELP_DIR;
> > +      else if (ftype == F2FS_FT_SYMLINK)
> > +        type = GRUB_FSHELP_SYMLINK;
> > +      else if (ftype == F2FS_FT_REG_FILE)
> > +        type = GRUB_FSHELP_REG;
> > +
> > +      fdiro->data = ctx->data;
> > +      fdiro->ino = grub_le_to_cpu32 (ctx->dentry[i].ino);
> > +      fdiro->inode_read = 0;
> > +
> > +      if (ctx->hook (filename, type, fdiro, ctx->hook_data))
> > +        return 1;
> > +
> > +      i += (name_len + F2FS_SLOT_LEN - 1) / F2FS_SLOT_LEN;
> > +    }
> > +    return 0;
> > +}
> > +
> ...
> > +
> > +static int
> > +grub_f2fs_iterate_dir (grub_fshelp_node_t dir,
> > +			 grub_fshelp_iterate_dir_hook_t hook, void *hook_data)
> > +{
> > +  struct grub_fshelp_node *diro = (struct grub_fshelp_node *) dir;
> > +  struct grub_f2fs_inode *inode;
> > +  struct grub_f2fs_dir_iter_ctx ctx = {
> > +    .data = diro->data,
> > +    .hook = hook,
> > +    .hook_data = hook_data
> > +  };
> > +  grub_off_t fpos = 0;
> > +
> > +  if (!diro->inode_read)
> > +    {
> > +      grub_f2fs_read_node (diro->data, diro->ino, &diro->inode);
> > +      if (grub_errno)
> > +	return 0;
> > +    }
> > +
> > +  inode = &diro->inode.i;
> > +
> > +  if (__inode_flag_set (inode, FI_INLINE_DENTRY))
> > +    return grub_f2fs_iterate_inline_dir (inode, &ctx);
> > +
> > +  while (fpos < __i_size (inode))
> > +    {
> > +      struct grub_f2fs_dentry_block *de_blk;
> > +      char *buf;
> > +
> > +      buf = grub_zalloc (F2FS_BLKSIZE);
> > +      if (!buf)
> > +        return 0;
> > +
> > +      grub_f2fs_read_file (diro, 0, 0, fpos, F2FS_BLKSIZE, buf);
> > +      if (grub_errno)
> > +        {
> > +          grub_free (buf);
> > +          return 0;
> > +        }
> > +
> > +      de_blk = (struct grub_f2fs_dentry_block *) buf;
> > +
> > +      ctx.bitmap = (grub_uint32_t *) de_blk->dentry_bitmap;
> > +      ctx.dentry = de_blk->dentry;
> > +      ctx.filename = de_blk->filename;
> > +      ctx.max = NR_DENTRY_IN_BLOCK;
> > +
> > +      if (grub_f2fs_check_dentries (&ctx))
> > +        return 1;
> 
> memory leak
> 
> > +
> > +      grub_free (buf);
> > +
> > +      fpos += F2FS_BLKSIZE;
> > +    }
> > +  return 0;
> > +}
> > +
> ...
> > +static grub_err_t
> > +grub_f2fs_dir (grub_device_t device, const char *path,
> > +		 grub_fs_dir_hook_t hook, void *hook_data)
> > +{
> > +  struct grub_f2fs_dir_ctx ctx = {
> > +    .hook = hook,
> > +    .hook_data = hook_data
> > +  };
> > +  struct grub_fshelp_node *fdiro = 0;
> > +
> > +  grub_dl_ref (my_mod);
> > +
> > +  ctx.data = grub_f2fs_mount (device->disk);
> > +  if (!ctx.data)
> > +    goto fail;
> > +
> > +  grub_fshelp_find_file (path, &ctx.data->diropen, &fdiro,
> > +			 grub_f2fs_iterate_dir, grub_f2fs_read_symlink,
> > +			 GRUB_FSHELP_DIR);
> > +  if (grub_errno)
> > +    goto fail;
> > +
> > +  grub_f2fs_iterate_dir (fdiro, grub_f2fs_dir_iter, &ctx);
> > +
> > +fail:
> > +  if (fdiro != &ctx.data->diropen)
> > +    grub_free (fdiro);
> > +  if (ctx.data)
> > +    grub_free (ctx.data->nat_bitmap);
> 
> Triple free :)
> 
> > +  grub_free (ctx.data);
> > +  grub_dl_unref (my_mod);
> > +  return grub_errno;
> > +}
> > +
> > +
> > +/* Open a file named NAME and initialize FILE.  */
> > +static grub_err_t
> > +grub_f2fs_open (struct grub_file *file, const char *name)
> > +{
> > +  struct grub_f2fs_data *data = NULL;
> > +  struct grub_fshelp_node *fdiro = 0;
> > +
> > +  grub_dl_ref (my_mod);
> > +
> > +  data = grub_f2fs_mount (file->device->disk);
> > +  if (!data)
> > +    goto fail;
> > +
> > +  grub_fshelp_find_file (name, &data->diropen, &fdiro,
> > +			 grub_f2fs_iterate_dir, grub_f2fs_read_symlink,
> > +			 GRUB_FSHELP_REG);
> > +  if (grub_errno)
> > +    goto fail;
> > +
> > +  if (!fdiro->inode_read)
> > +    {
> > +      grub_f2fs_read_node (data, fdiro->ino, &fdiro->inode);
> > +      if (grub_errno)
> > +	goto fail;
> > +    }
> > +
> > +  grub_memcpy (data->inode, &fdiro->inode, F2FS_BLKSIZE);
> sizeof (*data->inode)? Or they can be different?
> 
> > +  grub_free (fdiro);
> > +
> > +  file->size = __i_size (&(data->inode->i));
> > +  file->data = data;
> > +  file->offset = 0;
> > +
> > +  return 0;
> > +
> > +fail:
> > +  if (fdiro != &data->diropen)
> > +    grub_free (fdiro);
> > +  if (data)
> > +    grub_free (data->nat_bitmap);
> 
> Again.
> 
> > +  grub_free (data);
> > +
> > +  grub_dl_unref (my_mod);
> > +
> > +  return grub_errno;
> > +}
> > +
> > +static grub_ssize_t
> > +grub_f2fs_read (grub_file_t file, char *buf, grub_size_t len)
> > +{
> > +  struct grub_f2fs_data *data = (struct grub_f2fs_data *) file->data;
> > +
> > +  return grub_f2fs_read_file (&data->diropen,
> > +				file->read_hook, file->read_hook_data,
> > +				file->offset, len, buf);
> > +}
> > +
> > +static grub_err_t
> > +grub_f2fs_close (grub_file_t file)
> > +{
> > +  struct grub_f2fs_data *data = (struct grub_f2fs_data *) file->data;
> > +
> > +  if (data)
> > +    grub_free (data->nat_bitmap);
> 
> Again.
> 
> > +  grub_free (data);
> > +
> > +  grub_dl_unref (my_mod);
> > +
> > +  return GRUB_ERR_NONE;
> > +}
> > +
> > +static grub_err_t
> > +grub_f2fs_label (grub_device_t device, char **label)
> > +{
> > +  struct grub_f2fs_data *data;
> > +  grub_disk_t disk = device->disk;
> > +
> > +  grub_dl_ref (my_mod);
> > +
> > +  data = grub_f2fs_mount (disk);
> > +  if (data)
> > +    {
> > +      *label = grub_zalloc (sizeof (data->sblock.volume_name));
> > +      grub_utf16_to_utf8 ((grub_uint8_t *) (*label),
> 
> malloc failure check?
> 
> > +				data->sblock.volume_name, 512);
> 
> Where 512 comes from? Should it not be sizeof
> (data->sblock.volume_name) as well?
> 
> > +    }
> > +  else
> > +    *label = NULL;
> > +
> > +  if (data)
> > +    grub_free (data->nat_bitmap);
> 
> Again.
> 
> > +  grub_free (data);
> > +  grub_dl_unref (my_mod);
> > +  return grub_errno;
> > +}
> > +
> ...
> > diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
> > index e9e85c2..acc35cc 100644
> > --- a/tests/util/grub-fs-tester.in
> > +++ b/tests/util/grub-fs-tester.in
> > @@ -36,7 +36,7 @@ case x"$fs" in
> >  	MINLOGSECSIZE=8
> >  	    #  OS LIMITATION: It could go up to 32768 but Linux rejects sector sizes > 4096
> >  	MAXLOGSECSIZE=12;;
> > -    xxfs)
> > +    xxfs|xf2fs)
> >  	MINLOGSECSIZE=9
> >    	    # OS LIMITATION: GNU/Linux doesn't accept > 4096
> >  	MAXLOGSECSIZE=12;;
> > @@ -135,6 +135,10 @@ for ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> >  	    fi
> >  	    MAXBLKSIZE=4096
> >  	    ;;
> > +	xf2fs)
> > +	    MINBLKSIZE=$SECSIZE
> > +		# OS Limitation: GNU/Linux doesn't accept > 4096
> > +	    MAXBLKSIZE=4096;;
> >  	xsquash*)
> >  	    MINBLKSIZE=4096
> >  	    MAXBLKSIZE=1048576;;
> > @@ -256,7 +260,9 @@ for ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> >  	    # FS LIMITATION: btrfs label is at most 255 UTF-8 chars
> >  		x"btrfs"*)
> >  		    FSLABEL="grub_;/testé莭莽😁киритi urewfceniuewruevrewnuuireurevueurnievrewfnerfcnevirivinrewvnirewnivrewiuvcrewvnuewvrrrewniuerwreiuviurewiuviurewnuvewnvrenurnunuvrevuurerejiremvreijnvcreivire nverivnreivrevnureiorfnfrvoeoiroireoireoifrefoieroifoireoif";;
> > -
> > +	    # FS LIMITATION: btrfs label is at most 512 UTF-16 chars
> 
> F2FS, not btrfs
> 
> > +		x"f2fs")
> > +		    FSLABEL="grub_;/testjaegeuk kim 
> 
> Could you leave initial part in place? This includes some funny UNICODE
> characters for a reason, actually. Unless this is not possible with
> f2fs?
> 
> f2fsaskdfjkasdlfajskdfjaksdjfkjaskjkjkzjkjckzjvkcjkjkjekqjkwejkqwrlkasdfjksadjflaskdhzxhvjzxchvjzkxchvjkhakjsdhfjkhqjkwehrjkhasjkdfhjkashdfjkhjzkxhcjkvzhxcjkvhzxjchvkzhxckjvhjzkxchvjkhzjkxchvjkzhxckjvhzkxjchvkjzxhckjvzxcjkvhjzxkchkvjhzxkjcvhjkhjkahsjkdhkjqhwekrjhakjsdfhkjashdkjzhxcvjkhzxcvzxcvggggggggggf";;
> >  	    # FS LIMITATION: exfat is at most 15 UTF-16 chars
> >  		x"exfat")
> >  		    FSLABEL="géт ;/莭莽😁кир";;
> > @@ -466,7 +472,7 @@ for ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> >  	    # FIXME: Not sure about BtrFS, NTFS, JFS, AFS, UDF and SFS. Check it.
> >  	# FS LIMITATION: as far as I know those FS don't store their last modification date.
> >  		x"jfs_caseins" | x"jfs" | x"xfs"| x"btrfs"* | x"reiserfs_old" | x"reiserfs" \
> > -		    | x"bfs" | x"afs" \
> > +		    | x"bfs" | x"afs" | x"f2fs" \
> >  		    | x"tarfs" | x"cpio_"* | x"minix" | x"minix2" \
> >  		    | x"minix3" | x"ntfs"* | x"udf" | x"sfs"*)
> >  		    NOFSTIME=y;;
> > @@ -745,6 +751,8 @@ for ((LOGSECSIZE=MINLOGSECSIZE;LOGSECSIZE<=MAXLOGSECSIZE;LOGSECSIZE=LOGSECSIZE +
> >  		    MOUNTDEVICE="/dev/mapper/grub_test-testvol"
> >  		    MOUNTFS=ext2
> >  		    "mkfs.ext2" -L "$FSLABEL" -q "${MOUNTDEVICE}"  ;;
> > +		xf2fs)
> > +		    "mkfs.f2fs" -l "$FSLABEL" -q "${LODEVICES[0]}" ;;
> >  		xnilfs2)
> >  		    "mkfs.nilfs2" -L "$FSLABEL" -b $BLKSIZE  -q "${LODEVICES[0]}" ;;
> >  		xext2_old)

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel