Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

littlefs: fix EINVAL on path operations with littlefs 2.10.0 #15334

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Dec 25, 2024

Summary

littlefs 2.10.0 and later rejects empty paths. use "/" instead.

Impact

Testing

tested with esp32s3-devkit:toywasm.

littlefs 2.10.0 and later rejects empty paths.  use "/" instead.

tested with esp32s3-devkit:toywasm.
@github-actions github-actions bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Dec 25, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 25, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial information.

Here's what's missing:

  • Summary: Needs more detail. Why does littlefs 2.10.0+ reject empty paths? What was the problem this caused in NuttX? What code was changed to use "/" instead? (e.g., "LittleFS 2.10.0 and later reject empty paths when mounting, causing mount failures in NuttX when an empty string was passed to the mount function. This change modifies the xxx function in yyy to pass "/" as the mount point path if an empty string is provided.")
  • Impact: This section is entirely empty. All the "Impact on..." questions need to be answered (even if the answer is NO). For example:
    • Impact on user: NO
    • Impact on build: NO
    • Impact on hardware: NO
    • Impact on documentation: YES (If documentation needs updating to reflect this change or if the PR includes documentation updates.)
    • Impact on security: NO (Unless there's a security implication related to using "/" vs. an empty string)
    • Impact on compatibility: Potentially YES - explain if this change affects backward compatibility with apps using older versions of littlefs or forward compatibility with future versions.
  • Testing: More detail is required. "esp32s3-devkit:toywasm" is not enough information. Be specific about the OS, toolchain used on your build host. For your target, specify the NuttX configuration used (e.g., nsh). Crucially, you haven't provided any actual logs demonstrating the problem before the change and the successful operation after the change. Show the error you were seeing before and the lack of error after. Simply stating "tested" isn't sufficient.

In short, the PR needs to be much more descriptive and provide evidence of the problem and the solution's effectiveness. Without the missing information, reviewers cannot adequately assess the change.

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: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants