nuttxpr commented on PR #14924: URL: https://github.com/apache/nuttx/pull/14924#issuecomment-2496951307
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does *not* fully meet the NuttX requirements, though it's on the right track. Here's a breakdown of what's missing and how it could be improved: **Missing/Insufficient Information:** * **Summary:** * **Why is the change *necessary*?** "sscanf will do strlen firsttime, and then meminstream" doesn't explain *why* the existing behavior is a problem. Explain the bug or deficiency this fixes. Why is relying on libc's EOF definition incorrect? What are the negative consequences of the current implementation? * **What functional part of the code is being changed?** Be specific. Which files/functions/modules are affected? E.g., "Changes the implementation of `getc` within the `lib/stdio` directory to use `errno` for EOF detection." * **How does the change *exactly* work?** More detail is needed. "use errno to handle the stream getc EOF case" is too vague. Explain the mechanism used to set and check `errno`. * **Impact:** While some impact is mentioned, use "YES" and "NO" explicitly and elaborate more where necessary. * **Is new feature added? Is existing feature changed?** State this clearly. It sounds like an existing feature is being changed (bug fix). * **Impact on user:** If `sscanf` now behaves differently (even if it's fixing a bug), the user *might* need to adapt. Explain how the behavior changes and under what circumstances. * **Impact on documentation:** If this changes behavior, the documentation likely needs updating. State "YES" and specify what needs to be documented. * **Testing:** * **Build Host(s):** Be specific. "CI-test" isn't sufficient. Provide details about the CI environment (OS, compiler, etc.). Also provide details about your "local sim" setup. * **Target(s):** Be specific about the architecture and configuration of the simulator used. e.g., "sim:qemu-x86_64" or "sim:nsh." * **Testing logs:** Provide actual logs, both before and after the change, demonstrating the incorrect behavior and the fix. These logs should show the problem and how the change addresses it. "CI-test" is not a log. **Example of an Improved PR Description:** ``` ## Summary The current implementation of `getc` for memory streams (`meminstream`) in `lib/stdio` incorrectly relies on the libc definition of EOF, which can lead to unexpected behavior, particularly with `sscanf`. This was highlighted by a bug (#14778) where `sscanf` with the format string `" %4c%n"` failed and by incorrect EOF handling when `INT_MAX` is encountered. This change modifies `getc` to correctly use `errno` to signal end-of-file conditions for memory streams, ensuring consistent behavior across different stream types and resolving the aforementioned issues. ## Impact * Is new feature added? NO * Is existing feature changed? YES. This changes the behavior of `getc` for memory streams and consequently affects functions like `sscanf` that use it. * Impact on user: YES. Code that relies on the previous, incorrect EOF behavior of `meminstream` with `getc` might need to be adjusted. This primarily affects cases where `INT_MAX` was encountered in the stream, or formats like `" %4c%n"` in `sscanf`. * Impact on build: NO * Impact on hardware: NO * Impact on documentation: YES. The documentation for `getc` and `meminstream` should be updated to reflect the corrected EOF handling using `errno`. * Impact on security: NO * Impact on compatibility: YES. This change fixes a bug and improves consistency. While it's a positive change, it could theoretically affect code that inadvertently relied on the previous incorrect behavior. * Anything else to consider? No. ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0 * Target(s): sim:qemu-x86_64 (nsh configuration) Testing logs before change: ``` nsh> help scanftest scanftest scanftest 1234 5678 "abcd efgh" sscanf: abcd 5678 1234 (incorrect output - should be 1234 5678 abcd) ``` Testing logs after change: ``` nsh> help scanftest scanftest scanftest 1234 5678 "abcd efgh" sscanf: 1234 5678 abcd (correct output) ``` *(Include more relevant test cases, demonstrating the `INT_MAX` issue and the fix)* ``` ``` ``` ``` -- 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
