You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@nuttx.apache.org by "GUIDINGLI (via GitHub)" <gi...@apache.org> on 2024/10/10 09:26:15 UTC

[PR] fs poll optimize [nuttx]

GUIDINGLI opened a new pull request, #14039:
URL: https://github.com/apache/nuttx/pull/14039

   ## Summary
   
   fs poll optimize:
   
   fs: Set the poll return value according to the man manual
       
       The man manual describes that poll only has EFAULT, EINTR, EINVAL, and ENOMEM return values.
       If a file returns an error, the POLLERR event should be set and OK should be returned
       https://man7.org/linux/man-pages/man2/poll.2.html
       
       When using libuv to poll the socket, the socket poll returned an EBUSY error, causing libuv to abort.
       
       The expected logic should be to return OK, allowing libuv to notify the event listener that the POLLERR event occurred.
       
   fs/vfs: result_independent_of_operands
       
       timeout is a type of int
       in MSEC2TICK, timeout may be multiplied by USEC_PER_MSEC
       and the result may be out of range of int
       Besides, this will induce following ERROR while running simulator:
       vfs/fs_poll.c:498:38: runtime error: signed integer overflow:
       19768268 * 1000 cannot be represented in type 'int'
       
   
   ## Impact
   
   FS poll
   
   ## Testing
   
   sim & qemu
   
   


-- 
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] fs poll optimize [nuttx]

Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.

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

[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)

**No, this PR does not fully meet the NuttX requirements.**

**Missing Information:**

* **Summary:**
* Lacks a clear explanation of *why* the change is necessary. While the description mentions issues with `libuv` and potential integer overflows, it doesn't clearly articulate the problem being solved and its impact.
* Needs to be more specific about the functional part of the code being changed (e.g., which files, functions).
* **Impact:**
* The impact section is incomplete. It only mentions "FS poll" but doesn't address all the required points:
* Is this a new feature, a bug fix, or an optimization?
* What is the impact on users? Will they need to adapt to this change?
* What is the impact on documentation? Is an update required?
* Are there any security implications?
* Are there any backward or forward compatibility concerns?
* **Testing:**
* **Insufficient Detail:** Simply stating "sim & qemu" is not enough.
* Specify the host operating system, CPU architecture, and compiler used.
* Provide specific details about the simulator configurations.
* For QEMU, specify the target architecture and board configuration.
* **Missing Logs:** The "Testing logs before change" and "Testing logs after change" sections are empty. Provide actual log output demonstrating the issue before the change and the improvement after the change.

**Recommendations for Improvement:**

1. **Clarify the Problem:** In the Summary, clearly state the problem being solved. Explain why the existing `poll` implementation is incorrect according to the `man` page and how it negatively impacts `libuv`.
2. **Explain the Solution:** Provide more detail on *how* the code changes address the problem. What specific modifications are made to `fs_poll.c`? How does `result_independent_of_operands` relate to the overall fix?
3. **Complete the Impact Section:** Address all the points listed in the requirements. Be specific about any potential compatibility issues or required changes for users.
4. **Provide Detailed Testing Information:** Include:
* Host OS, CPU, and compiler details.
* Simulator/QEMU configurations.
* **Actual log output** before and after the change to demonstrate the fix.

By providing this missing information, your PR will be much stronger and easier for the NuttX maintainers to 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] fs poll optimize [nuttx]

Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.

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


-- 
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