grub-devel
[Top][All Lists]
Advanced

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]