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

update stream relative, fix mtd&blk stream readback before block update. decrese int use in stream to handle FS_LARGEFILE #14778

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Nov 14, 2024

Summary

int is not reliable when we use same code for cross platform.
replace with off_t,size_t,ssize_t should prefer.

offset int -> offset, len int -> size_t, ret int -> ssize_t

Also we should readback sector/block before operate to new sector/block,
for user only update partial case.

Impact

stream will able to support FS_LARGEFILE better cover >4GB devices.
if user use blk/mtd stream to update partial, the old data will be kept back.

Testing

CI-test, local arm-v8m prject, qemu-armv8a, ubutun sim:nsh

@github-actions github-actions bot added Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Nov 14, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 14, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although the provided information is minimal. It would be stronger with more detail. Here's a breakdown:

Strengths:

  • Clear Summary of "Why": Explains the motivation for the change (cross-platform compatibility and partial update handling).
  • Functional Change Description: Identifies the affected code (int to off_t/size_t/ssize_t) and the readback mechanism.
  • Impact on User/Features: Briefly mentions improved large file support and data preservation.
  • Testing Performed: Lists tested platforms (CI, ARMv8-M, qemu, Ubuntu sim).

Weaknesses & Missing Information:

  • No Issue References: Missing links to related NuttX issues. Even if there isn't a directly related issue, creating one beforehand is good practice.
  • Impact Details Lacking: The "Impact" section needs significant expansion. While "YES" is stated for user and feature impact, the descriptions are very brief. More details are needed for all impact categories (build, hardware, documentation, security, compatibility). Be explicit about any potential breaking changes. For example, does the change to size_t and off_t affect any existing drivers or applications? If documentation is required, state whether it's included in the PR.
  • Vague Testing Logs: Providing actual log snippets is crucial. "CI-test" is insufficient – link to the CI run. Include specific commands used and relevant output demonstrating the issue before the change and the corrected behavior afterward. Quantify the improvements if possible (e.g., "Successfully wrote a 5GB file," "Partial update preserved data from previous sectors X to Y").
  • Build Host Details Missing: Specify your development environment in the "Testing" section as requested by the template (OS, CPU, compiler versions).

Recommendation:

While the core elements are present, this PR description needs more detail to ensure thorough review and avoid potential problems. Expand the "Impact" and "Testing" sections considerably, providing concrete examples and data. Add issue references. A more detailed PR will increase confidence in the changes and streamline the review process.

@xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 merged commit f53f0ec into apache:master Nov 14, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Board: arm 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