[PR] sched/wqueue: fix work_notifier_setup() false failure [nuttx]

["kk-thrane (via GitHub)" <gi...@apache.org>]

kk-thrane opened a new pull request, #14629:
URL: https://github.com/apache/nuttx/pull/14629

   *Note: Please adhere to [Contributing Guidelines](https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md).*
   
   ## Summary
   
   Be consistent with the datatype used for the 'key'. As the API in
   in the .h file uses int, I chose to replace uint32_t with int
   everywhere.
   
   This change ensures that work_notifier_setup() will not return a
   negative value indicating error to the caller, when in fact the
   notifier was correctly setup. This would happen when
   work_notifier_setup() had been called 0x8000000 times, and that
   many keys allocated.
   
   ## Impact
   
   work_notifier_setup() is used in TCP, UDP, Netlink and IOB code.
   
   
   


-- 
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-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] sched/wqueue: fix work_notifier_setup() false failure [nuttx]

["nuttxpr (via GitHub)" <gi...@apache.org>]

nuttxpr commented on PR #14629:
URL: https://github.com/apache/nuttx/pull/14629#issuecomment-2454047834

   [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
   
   __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.
   
   
   
   Yes, this PR appears to mostly meet the NuttX requirements, but it is missing some critical information.
   
   Here's a breakdown of what's good and what needs improvement:
   
   **Strengths:**
   
   * **Clear Summary:**  The summary explains the "why," "what," and "how" of the change concisely.  The reasoning behind changing `uint32_t` to `int` is well-articulated.
   * **Impact Description (Partial):** The PR identifies affected areas (TCP, UDP, Netlink, IOB).
   
   **Weaknesses:**
   
   * **Missing Issue References:**  If this relates to a reported issue, link the relevant NuttX or NuttX Apps issue.
   * **Incomplete Impact Assessment:**  The impact section needs substantial expansion.  Specifically:
       * **Is new feature added? Is existing feature changed?**  Answer this directly. This is a bug fix and potentially a behavior change, so clarify that.
       * **Impact on user:** Even if the answer is NO, briefly explain why.  If an application was relying on the buggy behavior (unlikely but possible), this would be a breaking change.
       * **Impact on build, hardware, documentation, security, compatibility:** Explicitly state "NO" for each and provide a brief justification if necessary. For example, "Impact on build: NO (only code changes within existing files)" or "Impact on security: YES (prevents potential denial of service due to integer overflow)."
   * **Missing Testing Details:**
       * **Build Host(s):**  Specify the OS, CPU architecture, and compiler used for testing.
       * **Target(s):** Provide specifics about the target architecture, board, and configuration used.
       * **Testing Logs:** Include actual logs demonstrating the issue before the change and the corrected behavior after the change. This is the most critical missing piece.  Show how the previous behavior was problematic and how the change fixes it.  Ideally, include a test case that triggers the original bug.
   
   
   **Recommendation:**
   
   Before merging, this PR needs the following additions:
   
   1. **Link any related issues.**
   2. **Complete the Impact assessment** with explicit "NO" or "YES" answers and justifications.  Be thorough.
   3. **Provide detailed testing information,** including build host and target details, and, most importantly, **testing logs** that demonstrate the problem and the solution.
   
   
   By addressing these points, the PR will fully comply with the NuttX requirements and be ready for review.
   


-- 
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-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] sched/wqueue: fix work_notifier_setup() false failure [nuttx]

["xiaoxiang781216 (via GitHub)" <gi...@apache.org>]

xiaoxiang781216 merged PR #14629:
URL: https://github.com/apache/nuttx/pull/14629


-- 
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-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org