nuttxpr commented on PR #2870: URL: https://github.com/apache/nuttx-apps/pull/2870#issuecomment-2507872566
[**\[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. Here's why and how to improve it: **Summary:** * **Insufficient detail on "what functional part is being changed."** While it mentions `mbedtls_config.h` and patches, it lacks specificity. Which modules within MbedTLS are affected? Are these changes related to specific algorithms, ciphersuites, or features? Be more explicit. * **Unclear "how the change exactly works."** Saying "updates ... according to `git diff`" isn't helpful. Summarize the *meaning* of those diffs. What specific configurations are changed? What are the implications of those configuration changes? What was the problem with patch 0002, and how did manual application resolve it? * **Missing issue references.** If this relates to a security update, there's *almost certainly* an upstream MbedTLS advisory or CVE. Link to it. If not for a specific vulnerability, at least link to the MbedTLS release notes. **Impact:** * **Downplaying user impact.** "Check your `CONFIG_MBEDTLS_*` settings" isn't helpful. Users need to know *which* settings might have changed and *why* they might need to adjust them. What are the potential consequences of *not* checking these settings? * **Missing specifics on other impacts.** "Seems to work fine" is not acceptable. If there are security implications, be precise about which vulnerabilities are mitigated. For compatibility, explain potential issues with applications built against the older MbedTLS version. **Testing:** * **Insufficient testing.** "Builds fine" and "seems to work fine" on one architecture is not enough. Ideally, test on multiple architectures and boards. At a minimum, explain why testing was limited to RISC-V. * **Missing logs.** The template explicitly requests "Testing logs before change" and "Testing logs after change." These are crucial for demonstrating that the changes have the intended effect and don't introduce regressions. Include relevant output from tests, or at least examples of applications using MbedTLS before and after the change. * **Lack of detail on the build host.** Specify the OS, compiler version, etc. used for the build host. **How to improve the PR:** 1. **Elaborate on the summary.** Detail the specific MbedTLS modules and functionality affected. Explain the changes introduced by the patches, including the issue with patch 0002. 2. **Specify the impact.** List the affected `CONFIG_MBEDTLS_*` options and describe how users should adapt. Detail the security implications, referencing any relevant CVEs or advisories. Explain any compatibility concerns. 3. **Expand testing.** Test on multiple architectures if feasible. At minimum, justify the limited testing scope. Include relevant test logs demonstrating the changes' effects. Provide details about the build host environment. 4. **Provide clear links to relevant upstream resources:** MbedTLS release notes, security advisories, commit hashes, etc. By addressing these points, the PR will meet the NuttX requirements and be much easier for reviewers to evaluate. -- 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]
