nuttxpr commented on PR #2943: URL: https://github.com/apache/nuttx-apps/pull/2943#issuecomment-2588909148
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description mostly meets the NuttX requirements but could be improved. Here's a breakdown: **Strengths:** * **Clear Summary of Changes:** The summary clearly states what was changed and why. The use of bullet points and references to related issues/PRs is good. * **Impact Section Started:** Addressing the impact is started, but needs more detail. * **Testing Information Provided:** The testing section includes the host, target, config, commands used, and results. This is excellent. **Weaknesses:** * **Summary Lacks "How":** While the *what* is clear, the summary doesn't fully explain *how* `lib_get_tempbuffer()` saves stack space. Explain the mechanism briefly (e.g., it allocates buffers from the heap instead of the stack). * **Incomplete Impact Assessment:** The Impact section is too brief. It lists affected components but doesn't fully address all the required points. Every "YES" needs a description. For each "NO", explicitly state "NO". Example for "Impact on user": "YES, users of NSH will see slightly reduced heap space availability but improved stack usage." (Even if seemingly minor, describe it). * **Missing "Before" and "After" Logs in Testing Section:** The tables show the stack usage before and after the test command, but the actual console output/logs from the tests are missing. * **Unclear CI Status:** Mentioning "CI" is insufficient. State whether CI passed or provide a link to the CI run. **Suggested Improvements:** ## Summary * **Why:** NSH commands often use large buffers on the stack, potentially leading to stack overflows, especially with deeply nested pipelines. Using `lib_get_tempbuffer()` allocates these buffers from the heap, reducing stack usage and mitigating this risk. * **What:** This change modifies the NSH library and related applications to use `lib_get_tempbuffer()` for buffer allocation. It also replaces `CONFIG_NSH_LINELEN` with `LINE_MAX` for consistency. * **How:** Instead of declaring large character arrays on the stack, `lib_get_tempbuffer()` is used to dynamically allocate memory from the heap for these buffers. This memory is then freed when no longer needed. * **Related Issues/PRs:** (List any related issues/PRs) * **apps/nshlib:** Using `lib_get_tempbuffer()` to save stack space * **apps/**nshlib**: replace `CONFIG_NSH_LINELEN` to `LINE_MAX` * **apps/**system**: replace `CONFIG_NSH_LINELEN` to `LINE_MAX` (Picked from https://github.com/apache/nuttx-apps/pull/2918 by @anchao ) ## Impact * **New Feature Added:** NO * **Existing Feature Changed:** YES, NSH buffer management. * **Impact on User:** YES, users of NSH will see slightly reduced heap space availability due to dynamic buffer allocation, but significantly improved stack safety, reducing the risk of stack overflows. * **Impact on Build:** NO * **Impact on Hardware:** NO * **Impact on Documentation:** YES, documentation should be updated to reflect the use of `lib_get_tempbuffer()` and the change from `CONFIG_NSH_LINELEN` to `LINE_MAX`. * **Impact on Security:** YES, This change improves security by reducing the risk of stack overflow vulnerabilities. * **Impact on Compatibility:** NO * **Anything else to consider:** None ## Testing I confirm that changes are verified on local setup and works as intended: * **Build Host(s):** OS: Linux, CPU: Intel, Compiler: GCC (Provide version) * **Target(s):** arch: sim (esp32s3-devkit:adb), board: esp32s3-devkit, config: adb, `CONFIG_LINE_MAX=512` **Testing logs before change:** ``` # Actual command output and any relevant messages here, not just stack usage. ls ...output... cat | cat | cat ...output... free ...output... # Showing heap usage before ``` **Testing logs after change:** ``` # Actual command output and any relevant messages here ls ...output... cat | cat | cat ...output... free ...output... # Showing heap usage after ``` **Stack Usage Comparison:** (Include your tables as before) **CI Status:** [Link to CI run] or "CI passed successfully." By making these improvements, the PR description will be clearer, more complete, and better aligned with the NuttX requirements. This will make it easier for reviewers to understand and approve the changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
