fix: add poll() compatibility shim for Windows to fix VLC header compilation - #769
Closed
kt286 wants to merge 1 commit into
Closed
fix: add poll() compatibility shim for Windows to fix VLC header compilation#769kt286 wants to merge 1 commit into
kt286 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kt286 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @kt286. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's GuideAdds a Windows-only poll() compatibility shim and wires it into the build so VLC headers that use poll() compile on Windows, including linking the required Winsock library. Sequence diagram for Windows compat poll implementation using selectsequenceDiagram
participant Caller
participant poll
participant Winsock
Caller->>poll: poll(fds, nfds, timeout)
poll->>Winsock: FD_SET / FD_ZERO on fd_set
poll->>Winsock: select(0, &r, &w, &e, &tv)
Winsock-->>poll: ret
poll-->>Caller: ret with updated fds[i].revents
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Windows-only
pollshim currently never setsPOLLHUPorPOLLNVAL, so callers expecting full POSIXpoll()semantics may misbehave; consider mapping these based on closed/invalid sockets or theselect()return value where possible. - You only link
ws2_32whenWIN32 AND NOT MSVC, but the compatibilitypollinpoll.huses Winsock APIs for all Windows builds; ensurews2_32is linked for any configuration where this header can be force-included. - Using
add_compile_options(-include "${CMAKE_SOURCE_DIR}/src/compat/poll.h")hardcodes an absolute path into the compiler flags; you may want to switch to a relative include and ensure the directory is in the include path to avoid issues with out-of-tree builds or different source layouts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Windows-only `poll` shim currently never sets `POLLHUP` or `POLLNVAL`, so callers expecting full POSIX `poll()` semantics may misbehave; consider mapping these based on closed/invalid sockets or the `select()` return value where possible.
- You only link `ws2_32` when `WIN32 AND NOT MSVC`, but the compatibility `poll` in `poll.h` uses Winsock APIs for all Windows builds; ensure `ws2_32` is linked for any configuration where this header can be force-included.
- Using `add_compile_options(-include "${CMAKE_SOURCE_DIR}/src/compat/poll.h")` hardcodes an absolute path into the compiler flags; you may want to switch to a relative include and ensure the directory is in the include path to avoid issues with out-of-tree builds or different source layouts.
## Individual Comments
### Comment 1
<location path="src/compat/poll.h" line_range="31-40" />
<code_context>
+#define POLLNVAL 0x0008
+#endif
+
+static inline int poll(struct pollfd *fds, unsigned int nfds, int timeout) {
+ fd_set r, w, e;
+ FD_ZERO(&r); FD_ZERO(&w); FD_ZERO(&e);
+
+ SOCKET max_fd = 0;
+ for (unsigned int i = 0; i < nfds; i++) {
+ if (fds[i].events & POLLIN) FD_SET(fds[i].fd, &r);
+ if (fds[i].events & POLLOUT) FD_SET(fds[i].fd, &w);
+ FD_SET(fds[i].fd, &e);
+ if (fds[i].fd > max_fd) max_fd = fds[i].fd;
+ }
+
+ timeval tv;
+ tv.tv_sec = timeout / 1000;
+ tv.tv_usec = (timeout % 1000) * 1000;
+
+ int ret = select(0, &r, &w, &e, timeout >= 0 ? &tv : NULL);
+
+ for (unsigned int i = 0; i < nfds; i++) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle select() error return before inspecting fd_sets to avoid undefined behavior.
On Windows, when `select()` fails it returns `SOCKET_ERROR` and the `fd_set`s become undefined. The current logic still iterates over `fds` and derives `revents` when `ret < 0`, which can produce incorrect `POLLIN`/`POLLOUT`/`POLLERR` flags. Please short‑circuit on error (e.g. return immediately when `ret <= 0`), or at minimum guard the loop with `if (ret > 0)`.
</issue_to_address>
### Comment 2
<location path="src/compat/poll.h" line_range="35-40" />
<code_context>
+ fd_set r, w, e;
+ FD_ZERO(&r); FD_ZERO(&w); FD_ZERO(&e);
+
+ SOCKET max_fd = 0;
+ for (unsigned int i = 0; i < nfds; i++) {
+ if (fds[i].events & POLLIN) FD_SET(fds[i].fd, &r);
+ if (fds[i].events & POLLOUT) FD_SET(fds[i].fd, &w);
+ FD_SET(fds[i].fd, &e);
+ if (fds[i].fd > max_fd) max_fd = fds[i].fd;
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Remove unused max_fd or use it consistently for future non-Windows paths.
`max_fd` is calculated but never used in the `select()` call, so it’s currently dead code on Windows and may mislead readers into thinking it matters. Please either remove `max_fd` or clearly document why it’s unused, particularly if this code may be shared with non-Windows implementations later.
Suggested implementation:
```c
fd_set r, w, e;
FD_ZERO(&r); FD_ZERO(&w); FD_ZERO(&e);
```
```c
fd_set r, w, e;
FD_ZERO(&r); FD_ZERO(&w); FD_ZERO(&e);
for (unsigned int i = 0; i < nfds; i++) {
if (fds[i].events & POLLIN) FD_SET(fds[i].fd, &r);
if (fds[i].events & POLLOUT) FD_SET(fds[i].fd, &w);
FD_SET(fds[i].fd, &e);
}
#ifndef _COMPAT_POLL_H
#define _COMPAT_POLL_H
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
TAG Bot New tag: 7.0.66 |
…ilation - Add src/compat/poll.h compatibility header implementing poll() on Windows using select() - Modify CMakeLists.txt to force-include poll.h for Windows platform - Link ws2_32 library for poll() compatibility implementation fix: 添加 Windows 平台 poll() 兼容性实现，修复 VLC 头文件编译问题 - 添加 src/compat/poll.h 兼容性头文件，在 Windows 上基于 select() 实现 poll() 函数 - 修改 CMakeLists.txt，为 Windows 平台添加 poll.h 强制包含 - 链接 ws2_32 库以支持 poll() 兼容性实现
kt286
marked this pull request as draft
August 24, 2026 11:54
Contributor
Author
|
给 msys2 中的 vlc 提交了补丁，不需要处理了 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: 添加 Windows 平台 poll() 兼容性实现，修复 VLC 头文件编译问题
Summary by Sourcery
Add Windows poll() compatibility support so VLC headers compile successfully on the Windows build.
Bug Fixes:
Build: