nuttx icon indicating copy to clipboard operation
nuttx copied to clipboard

part 1 of romfs extend to enable format function

Open guohao15 opened this issue 1 year ago • 1 comments

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

guohao15 avatar Sep 14 '24 06:09 guohao15

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

lupyuen avatar Sep 18 '24 19:09 lupyuen

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!

JianyuWang0623 avatar Dec 05 '24 12:12 JianyuWang0623