Skip to content

Conversation

@guohao15
Copy link
Contributor

@guohao15 guohao15 commented Nov 18, 2024

Summary

This reverts commit ac6fff7.
This reverts commit c83e3f6.
This reverts commit 133db24.
This reverts commit 5c1248e.
This reverts commit 2cc850b.
This reverts commit 49cad84.

Remove the patch sets about writable romfs, since the writable feature will migrate to a new file system instead patching romfs inplace.

Impact

romfs , remove the code configed by FS_ROMFS_WRITEABLE

Testing

test about romfs

This reverts commit ac6fff7.
This reverts commit c83e3f6.
This reverts commit 133db24.
This reverts commit 5c1248e.
This reverts commit 2cc850b.
This reverts commit 49cad84.

Remove the patch sets about writable romfs

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 Nov 18, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 18, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details and context, making it difficult to review effectively.

Here's a breakdown of the issues and how to fix them:

Summary:

  • Insufficient explanation: Simply listing reverted commits doesn't explain why the revert is necessary. What problems did those commits introduce? What was the original intention of those commits, and why did they fail? What specific functionality is being restored by this revert? Explain the "writable romfs" feature and why removing it is beneficial.
  • Missing context: What use cases were affected by the buggy writable romfs implementation? This helps reviewers understand the impact and importance of the revert.
  • No issue references: Are there any related NuttX issues that prompted this revert? If so, link them here.

Impact:

  • Vague descriptions: Saying "romfs, remove the code configed by FS_ROMFS_WRITEABLE" isn't helpful. Be explicit. Will users notice any functional changes? Will any configurations need to be adjusted? Will any applications break? If the answer to any impact question is "YES", explain why and how. For example:
    • Impact on user: YES. Users who were relying on the writable romfs feature will no longer be able to modify files in the romfs filesystem. They will need to use a different filesystem (e.g., littlefs) if write access is required.
    • Impact on build: Possibly YES. If any build configurations depended on FS_ROMFS_WRITEABLE, they will need to be updated. Explain how.
  • Consider all impact categories: Even if the answer is "NO", explicitly state it for each category (e.g., "Impact on security: NO"). This shows you've considered each aspect.

Testing:

  • Insufficient detail: "test about romfs" is far too vague. What specific tests were run? Provide commands used, expected output, and actual output.
  • Missing logs: Include actual log output before and after the change, demonstrating the fix. Don't just leave placeholder comments.
  • Missing build/target information: Specify the exact build host (OS version, compiler version) and target (architecture, board, configuration) used for testing. For example:
    • Build Host: Linux (Ubuntu 22.04), GCC 11.3.0
    • Target: sim:nsh

Example of an improved Summary:

This PR reverts a series of commits (list commits here) that introduced a writable romfs implementation. This feature proved to be unstable and caused [describe the specific problems, e.g., data corruption, system crashes]. Reverting these commits restores the original read-only behavior of romfs. This addresses issue #[NuttX issue number, if applicable]. The writable romfs feature was intended to [explain the original goal], but due to [explain the reason for failure], it was deemed unsuitable. This revert removes the FS_ROMFS_WRITEABLE configuration option and associated code.

By providing more specific information and context in each section, you'll make it much easier for reviewers to understand your changes, assess their impact, and confidently approve your PR. Remember to follow the NuttX Contributing Guidelines carefully.

@anchao
Copy link
Contributor

anchao commented Nov 18, 2024

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Nov 18, 2024

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

The feature will migrate to a new file system to avoid affecting romfs.

@anchao
Copy link
Contributor

anchao commented Nov 18, 2024

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

The feature will migrate to a new file system to avoid affecting romfs.

could we remove the writeable feature after introducing the new file system? Developers who use this feature could find an alternative solution.

@xiaoxiang781216
Copy link
Contributor

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

The feature will migrate to a new file system to avoid affecting romfs.

could we remove the writeable feature after introducing the new file system? Developers who use this feature could find an alternative solution.

this implementation is in the partial state, and we decide to stop maintaining this version, so it isn't good to left it in this state to confuse people.

@anchao
Copy link
Contributor

anchao commented Nov 18, 2024

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

The feature will migrate to a new file system to avoid affecting romfs.

could we remove the writeable feature after introducing the new file system? Developers who use this feature could find an alternative solution.

this implementation is in the partial state, and we decide to stop maintaining this version, so it isn't good to left it in this state to confuse people.

but once the function is introduced into the community, you can't perceive whether any individual developer is using this feature. Do we have any better solution to give them if the revert this feature?

@xiaoxiang781216
Copy link
Contributor

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

The feature will migrate to a new file system to avoid affecting romfs.

could we remove the writeable feature after introducing the new file system? Developers who use this feature could find an alternative solution.

this implementation is in the partial state, and we decide to stop maintaining this version, so it isn't good to left it in this state to confuse people.

but once the function is introduced into the community, you can't perceive whether any individual developer is using this feature. Do we have any better solution to give them if the revert this feature?

As I said we will provide the full implementation after the new file system is stable.

@anchao
Copy link
Contributor

anchao commented Nov 18, 2024

Seems all functionality is controlled by FS_ROMFS_WRITEABLE. Could we keep this feature?

The feature will migrate to a new file system to avoid affecting romfs.

could we remove the writeable feature after introducing the new file system? Developers who use this feature could find an alternative solution.

this implementation is in the partial state, and we decide to stop maintaining this version, so it isn't good to left it in this state to confuse people.

but once the function is introduced into the community, you can't perceive whether any individual developer is using this feature. Do we have any better solution to give them if the revert this feature?

As I said we will provide the full implementation after the new file system is stable.

please provide before revert

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.

4 participants