[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] F2FS support
|
From: |
Andrei Borzenkov |
|
Subject: |
Re: [PATCH] F2FS support |
|
Date: |
Sat, 28 Mar 2015 10:31:55 +0300 |
В Tue, 24 Mar 2015 01:19:00 -0700
Jaegeuk Kim <address@hidden> пишет:
> * 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?
>
> 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 <address@hidden>
> + *
> + * 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)