[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v11 1/2] fs/erofs: Add support for EROFS
|
From: |
Gao Xiang |
|
Subject: |
Re: [PATCH v11 1/2] fs/erofs: Add support for EROFS |
|
Date: |
Fri, 17 May 2024 08:34:07 +0800 |
|
User-agent: |
Mozilla Thunderbird |
Hi Daniel,
On 2024/5/17 05:33, Daniel Kiper wrote:
On Fri, May 10, 2024 at 08:52:55AM +0800, Gao Xiang wrote:
...
In general patch LGTM except some nits...
Thanks for the comments. Let me address them.
---
Tested-by Link:
https://lists.gnu.org/archive/html/grub-devel/2024-05/msg00001.html
...
+struct grub_erofs_super
+{
+ grub_uint32_t magic;
+ grub_uint32_t checksum;
+ grub_uint32_t feature_compat;
+ grub_uint8_t log2_blksz;
+ grub_uint8_t sb_extslots;
May I ask you to align struct/union members in the following way?
grub_uint32_t magic;
grub_uint32_t checksum;
grub_uint32_t feature_compat;
grub_uint8_t log2_blksz;
grub_uint8_t sb_extslots;
grub_uint16_t root_nid;
grub_uint64_t inos;
grub_uint64_t build_time;
grub_uint32_t build_time_nsec;
...
Sorry, anyway, I'm not quite familiar with GRUB coding
styles.
It seems that using TABs in the middle of names and types.
Is my understanding correct? I will try to update like this.
Please do this for all/most structs and unions definitions below.
...
+
+static grub_err_t
+erofs_read_inode (struct grub_erofs_data *data, grub_fshelp_node_t node)
+{
+ union grub_erofs_inode *di;
+ grub_err_t err;
+ grub_uint16_t i_format;
+ grub_uint64_t addr = erofs_iloc (node);
+
+ di = (union grub_erofs_inode *) &node->inode;
+
+ err = grub_disk_read (data->disk, addr >> GRUB_DISK_SECTOR_BITS,
+ addr & (GRUB_DISK_SECTOR_SIZE - 1),
+ sizeof (struct grub_erofs_inode_compact), &di->c);
+ if (err != GRUB_ERR_NONE)
+ return err;
+
+ i_format = grub_le_to_cpu16 (di->c.i_format);
+ node->inode_type = (i_format >> EROFS_I_VERSION_BIT) & EROFS_I_VERSION_MASKS;
+ node->inode_datalayout = (i_format >> EROFS_I_DATALAYOUT_BIT) &
EROFS_I_DATALAYOUT_MASKS;
+
+ switch (node->inode_type)
+ {
+ case EROFS_INODE_LAYOUT_EXTENDED:
+ addr += sizeof (struct grub_erofs_inode_compact);
+ err = grub_disk_read (
+ data->disk, addr >> GRUB_DISK_SECTOR_BITS,
+ addr & (GRUB_DISK_SECTOR_SIZE - 1),
+ sizeof (struct grub_erofs_inode_extended) - sizeof (struct
grub_erofs_inode_compact),
+ (grub_uint8_t *) di + sizeof (struct grub_erofs_inode_compact));
This function call is unreadable. Please fix it. I am OK with lines
longer than 80 chars. So, first argument for the grub_disk_read() should
be immediately behind "(". Then other arguments may follow below
starting from the same column with the first argument.
grub_disk_read (data->disk, addr >> GRUB_DISK_SECTOR_BITS,
addr & (GRUB_DISK_SECTOR_SIZE - 1),
...
Okay, will fix.
...
+
+ /* file_size is checked by caller and cannot be zero, hence nblocks > 0 */
+ file_size = erofs_inode_file_size (node);
+ if (grub_add (file_size, blocksz - 1, &nblocks))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
You use "overflow is detected" message everywhere. It is not helpful.
I think (almost) every message should be different, e.g. "nblocks overflow"
or "invalid argument: %d..." or ...
Let me fix it as "nblocks overflow".
+ nblocks >>= node->data->sb.log2_blksz;
+ lastblk = nblocks - tailendpacking;
+
+ map->m_flags = EROFS_MAP_MAPPED;
+
+ /* no overflow as (lastblk <= nblocks) && (nblocks * blocksz <= UINT64_MAX -
blocksz + 1) */
+ if (map->m_la < (lastblk * blocksz))
+ {
+ if (grub_mul ((grub_uint64_t) grub_le_to_cpu32
(node->inode.e.i_u.raw_blkaddr), blocksz,
+ &map->m_pa) ||
Please move this to the line with grub_mul()...
Okay, will fix as
if (grub_mul ((grub_uint64_t) grub_le_to_cpu32 (node->inode.e.i_u.raw_blkaddr),
blocksz, &map->m_pa) ||
+ grub_add (map->m_pa, map->m_la, &map->m_pa))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
Again, this message does not help...
Will fix as "m_pa overflow".
+ if (grub_sub (lastblk * blocksz, map->m_la, &map->m_plen))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
To be precise, this is an underflow...
Will fix as "m_plen underflow".
+ }
+ else if (tailendpacking)
+ {
+ if (grub_add (erofs_iloc (node), erofs_inode_size (node), &map->m_pa) ||
+ grub_add (map->m_pa, erofs_inode_xattr_ibody_size (node), &map->m_pa)
||
+ grub_add (map->m_pa, map->m_la % blocksz, &map->m_pa))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
Please fix all these messages...
Thanks, will fix as "m_pa overflow".
+ if (grub_sub (file_size, map->m_la, &map->m_plen))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
Will fix as "m_plen overflow".
+
+ /* no overflow as map->m_plen <= UINT64_MAX - blocksz + 1 */
+ if (((map->m_pa % blocksz) + map->m_plen) > blocksz)
+ return grub_error (
+ GRUB_ERR_BAD_FS,
+ "inline data cross block boundary @ inode %" PRIuGRUB_UINT64_T,
+ node->ino);
Wrong wrapping as above. Please fix it.
Should I fix this in the same line?
Will try to fix.
+ }
+ else
+ return grub_error (GRUB_ERR_BAD_FS,
+ "invalid map->m_la=%" PRIuGRUB_UINT64_T
+ " @ inode %" PRIuGRUB_UINT64_T,
+ map->m_la, node->ino);
Ditto... Please fix similar wrapping everywhere.
Should I fix this in the same line?
Will try to fix.
+ map->m_llen = map->m_plen;
+ return GRUB_ERR_NONE;
+}
+
+static grub_err_t
+erofs_map_blocks_chunkmode (grub_fshelp_node_t node,
+ struct grub_erofs_map_blocks *map)
+{
+ grub_uint16_t chunk_format = grub_le_to_cpu16 (node->inode.e.i_u.c.format);
+ grub_uint64_t unit, pos, chunknr, blkaddr;
+ grub_uint8_t chunkbits;
+ grub_err_t err;
+
+ if (chunk_format & EROFS_CHUNK_FORMAT_INDEXES)
+ unit = sizeof (struct grub_erofs_inode_chunk_index);
+ else
+ unit = EROFS_BLOCK_MAP_ENTRY_SIZE;
+
+ chunkbits = node->data->sb.log2_blksz + (chunk_format &
EROFS_CHUNK_FORMAT_BLKBITS_MASK);
+ if (chunkbits > 63)
+ return grub_error (GRUB_ERR_BAD_FS, "invalid chunkbits %u @ inode %"
PRIuGRUB_UINT64_T,
+ chunkbits, node->ino);
+
+ chunknr = map->m_la >> chunkbits;
+
+ if (grub_add (erofs_iloc (node), erofs_inode_size (node), &pos) ||
+ grub_add (pos, erofs_inode_xattr_ibody_size (node), &pos))
Maybe in some cases it is worth checking overflow for every operation
separately and then print relevant messages.
I'm not sure since they are all steps to calculate the inode chunk map
physical offset (position) in bytes.
Maybe "chunkmap position overflow"?
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
+
+ /* pos = ALIGN_UP(pos, unit) */
+ if (grub_add (pos, unit - 1, &pos))
+ return grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow is detected");
I'd also update this as "chunkmap position overflow"?
+ pos &= ~(unit - 1);
May I ask you to add a macro which allows overflow detection, e.g.
ALIGN_UP_OVF().
This should go to a separate patch.
Not quite sure what's like? Could we handle this later?
+ /* no overflow for multiplication as chunkbits >= 9 and sizeof(unit) <= 8 */
Please start all comments with upper case and end with full stop.
Will try to fix them all.
...
+
+ if (!node->inode_loaded)
if (node->inode_loaded == false)
... and please fix similar things everywhere...
Okay, !node->inode_loaded is Linux-like style, I will try to fix what I find.
+ {
...
+
+static int
+erofs_iterate_dir (grub_fshelp_node_t dir, grub_fshelp_iterate_dir_hook_t hook,
+ void *hook_data)
+{
+ grub_uint64_t offset = 0, file_size;
+ grub_uint32_t blocksz = erofs_blocksz (dir->data);
+ grub_uint8_t *buf;
+ grub_err_t err;
+
+ if (!dir->inode_loaded)
Ditto.
Okay.
+ {
...
+ file_size = erofs_inode_file_size (dir);
+ buf = grub_malloc (blocksz);
+ if (!buf)
if (buf == NULL)
I know your version works but it is less readable. Please fix similar
checks everywhere...
Okay.
+ return 0;
+
...
+
+ fdiro = grub_malloc (sizeof (struct grub_fshelp_node));
+ if (!fdiro)
Ditto...
Okay..
Thanks for reviewing!
Thanks,
Gao Xiang
Daniel