Skip to content

Conversation

@guohao15
Copy link
Contributor

Summary

part 1 of romfs extend to enable format function

Impact

This is part 1 of a series of patchs.

To enhance the write capabilities of ROMFS, enabling it to perform simple file creation, writing, and deletion operations while maintaining compatibility with the original ROMFS format used in open-source ROMFS.

Feature is enabled by CONFIG_FS_ROMFS_WRITEABLE Kconfig
When CONFIG_FS_ROMFS_WRITEABLE is enabled
We can use -o rw to enable writable

Testing

cmoka_fs_test
cmoka_syscall_test

@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

The PR summary and impact descriptions provide a good start, but lack some specifics to fully meet NuttX requirements. Here's a breakdown and suggestions for improvement:

Summary - Needs Improvement:

  • Why change is necessary? You mention enhancing write capabilities, but not the underlying reason. Is this a user request? Does it address a specific limitation or bug? Be explicit about the "why".
  • What functional part is changed? "romfs extend" is vague. Mention specific files/modules within the ROMFS subsystem that are modified.
  • How does the change work? "enable format function" is the result, not the mechanism. Describe at a high-level the technical approach taken (e.g., new data structures, modifications to existing algorithms, etc.).

Impact - Mostly Good:

  • New feature added? Clearly stated, but briefly elaborate on the capabilities this new feature provides.
  • Impact on user? Mentioning CONFIG_FS_ROMFS_WRITEABLE and -o rw is good, but provide a short example of how a user would utilize this in practice.
  • Other impacts: While you mark most as "NO", a multi-part change like this likely has implications. Even if unchanged now, consider:
    • Build: Will later parts require build system changes? If unsure, acknowledge this.
    • Documentation: Part 1 likely needs documentation updates. If not included now, mention it's planned for a later part.

Testing - Needs Significant Improvement:

  • Build Hosts & Targets: Provide specifics, don't leave it generic. List OS, versions (compiler especially), and for targets the exact board:config combinations used.
  • Testing Logs: "cmoka_fs_test" is not useful. Include relevant snippets of logs demonstrating:
    • The issue BEFORE your change (proving it was a problem)
    • Successful operation AFTER the change
    • Ideally, test cases covering both normal usage AND error handling

Overall:

The PR is on the right track but needs more detail and concrete evidence of testing. Addressing the points above will make it much stronger.

change romfs_cachenode to find all headers

Signed-off-by: guohao15 <guohao15@xiaomi.com>
add sparelist api

Signed-off-by: guohao15 <guohao15@xiaomi.com>
fix for df cmd infomation of romfs

Signed-off-by: guohao15 <guohao15@xiaomi.com>
origoffset represent the offset of current node
offset represent the offset of the file when
the current node is a soft or hard link

Signed-off-by: guohao15 <guohao15@xiaomi.com>
@github-actions github-actions bot added Area: File System File System issues Size: L The size of the change in this PR is large labels Oct 8, 2024
guohao15 and others added 9 commits October 8, 2024 20:33
add romfs_mkfs to support autoformat && forceformat

Signed-off-by: guohao15 <guohao15@xiaomi.com>
and fix the minor style issue

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
to support the directory with more than 255 files

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
…tor boundary

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
and set volume size to 96 bytes in romfs_mkfs

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
add sem for write safe

Signed-off-by: guohao15 <guohao15@xiaomi.com>
@GUIDINGLI GUIDINGLI merged commit 38f0056 into apache:master Oct 11, 2024
@JianyuWang0623
Copy link
Contributor

The commit fs/romfs: Change the type of num from uint8_t to uint16_t fixs the case that when the number of romfs sub-node is greater than 256, the "num"("count" now) overflowed. e.g. the memory management was broken.
Debug with kasan:

kasan_report
/workspace/nuttx/mm/kasan/kasan.c:117
kasan_check_report
/workspace/nuttx/mm/kasan/kasan.c:190
__asan_store4_noabort
/workspace/nuttx/mm/kasan/kasan.c:314
romfs_cachenode
/workspace/nuttx/arch/arm/src/../../../fs/romfs/fs_romfsutil.c:428
romfs_cachenode
/workspace/nuttx/arch/arm/src/../../../fs/romfs/fs_romfsutil.c:484
romfs_fsconfigure
/workspace/nuttx/arch/arm/src/../../../fs/romfs/fs_romfsutil.c:773
nx_mount
/workspace/nuttx/arch/arm/src/../../../fs/mount/fs_mount.c:445
mount
/workspace/nuttx/arch/arm/src/../../../fs/mount/fs_mount.c:561
nsh_command
/workspace/nuttx/arch/arm/src/../../../../apps/nshlib/nsh_command.c:1247
nsh_execute
/workspace/nuttx/arch/arm/src/../../../../apps/nshlib/nsh_parse.c:847
nx_mount
/workspace/nuttx/arch/arm/src/../../../fs/mount/fs_mount.c:445
mount
/workspace/nuttx/arch/arm/src/../../../fs/mount/fs_mount.c:561
nsh_command
/workspace/nuttx/arch/arm/src/../../../../apps/nshlib/nsh_command.c:1247
nsh_execute
/workspace/nuttx/arch/arm/src/../../../../apps/nshlib/nsh_parse.c:847
nsh_parse
/workspace/nuttx/arch/arm/src/../../../../apps/nshlib/nsh_parse.c:2844
nsh_script

@xiaoxiang781216 @guohao15 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: File System File System issues Size: L The size of the change in this PR is large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants