RFC: Revised patchset to fix Bug 7929 (C&C 3 network does not work)
I realize it's been some time since I've submitted something for the networking problems with the C&C games, but I believe I've come up with something that resolves the major show-stopping issue that was discussed previously. Here is the original thread, for reference: http://www.winehq.org/pipermail/wine-devel/2009-October/079099.html Specifically, this set of patches is revised to address the problem of throwing away packets destined for the incorrect interface in a multi-threaded/multi-process environment. The approach used here is to create a global (named) POSIX.1-2001 semaphore for each socket with the form wine-ifacelock-FD , where "FD" is the Unix file descriptor corresponding to the socket at hand. These semaphores are only created for UDP sockets bound to a specific interface and are locked/unlocked by both the Wine user-space and the Wine server. These patches are against wine-1.3.3 and are split apart according to function. The most revised portions since the original set are 2/6 (server-side creation of semaphore), 3/6 (locking and unlocking), and 6/6 (server-side locking and unlocking). If I could get some feedback on these patches then I would greatly appreciate it, it'd be really nice to finally get this issue resolved. Thanks in advance! [1/6] ws2_32/tests: Add UDP broadcast tests: http://www.compholio.com/wine-kane/patches/2010-09-18/0001-ws2_32-tests-Add-... [2/6] server: Add mechanism for storing an interface ID with a socket: http://www.compholio.com/wine-kane/patches/2010-09-18/0002-server-Add-mechan... [3/6] ws2_32: Patch to selectively bind to interfaces while still allowing broadcast packets: http://www.compholio.com/wine-kane/patches/2010-09-18/0003-ws2_32-Patch-to-s... [4/6] ws2_32: Ensure select does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0004-ws2_32-Ensure-sel... [5/6] ws2_32: Ensure Async WSARecv does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0005-ws2_32-Ensure-Asy... [6/6] server: Ensure Async ReadFile does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0006-server-Ensure-Asy... Erich Hoover ehoover(a)mines.edu
A few broad comments: - There's enough #ifdef's to make this mostly unreadable. It's hard to think of the cases of no IP_PKTINFO availability, only compile time availability or full availability. Please just write a few wrapper functions for the #ifdef's that return sensible values if IP_PKTINFO is not available and then use them. I understand that in some places (i.e. WS2_send/WS2_recv) are supposed to be #ifdef'y but it may be easier to comment if you minimize the sheer number of combinations. - Pick a place to do the filtering and stick with it. You do some filtering in the server, some filtering in wine-userspace and some calls aren't filtered at all (see blocking recv's). I would suggest forwarding ReadFile/WriteFile to WSARecv/WSASend for sockets and then taking care of it in ws2_32 (we might end up needing this soon anyway for other annoying reasons). Even if your async reads are woken, you should be lenient enough in WS2_recv to return EWOULDBLOCK if no packets matching the interface are available. Since you don't do this now blocking recv's are broken. Also, FYI our *nix sockets are always non-blocking, you don't need MSG_DONTWAIT. - So what if an application likes to bind to an interface AND use IP_PKTINFO? - I don't think this is the right approach. Alexandre's comments from last year still apply. We would have to change the way we do sockets entirely; instead of just passing through to similar functions in *nix we would need to emulate a network stack (which is really what your patches crudely do). For example, now we wouldn't be able to use poll anywhere reliably on sockets. I don't think that's a direction Alexandre wants to go in. I think only one of two things will help us in this case - the kernel folks appease us or wineserver becomes a kernel module and we just do I/O from wineserver. Both of which are on schedule for around the time hell freezes over. - If you really want to get this to work consider writing something to LD_PRELOAD that emulates most of the network syscalls (+ read) with windows-style bind'ing correctly. I'd argue this may be easier and faster than putting the fix it into wine. This may be more user-friendly than having people compile wine themselves. Mike. On Sat, Sep 18, 2010 at 10:44 PM, Erich Hoover <ehoover(a)mines.edu> wrote:
I realize it's been some time since I've submitted something for the networking problems with the C&C games, but I believe I've come up with something that resolves the major show-stopping issue that was discussed previously. Here is the original thread, for reference: http://www.winehq.org/pipermail/wine-devel/2009-October/079099.html
Specifically, this set of patches is revised to address the problem of throwing away packets destined for the incorrect interface in a multi-threaded/multi-process environment. The approach used here is to create a global (named) POSIX.1-2001 semaphore for each socket with the form wine-ifacelock-FD , where "FD" is the Unix file descriptor corresponding to the socket at hand. These semaphores are only created for UDP sockets bound to a specific interface and are locked/unlocked by both the Wine user-space and the Wine server.
These patches are against wine-1.3.3 and are split apart according to function. The most revised portions since the original set are 2/6 (server-side creation of semaphore), 3/6 (locking and unlocking), and 6/6 (server-side locking and unlocking). If I could get some feedback on these patches then I would greatly appreciate it, it'd be really nice to finally get this issue resolved. Thanks in advance!
[1/6] ws2_32/tests: Add UDP broadcast tests: http://www.compholio.com/wine-kane/patches/2010-09-18/0001-ws2_32-tests-Add-... [2/6] server: Add mechanism for storing an interface ID with a socket: http://www.compholio.com/wine-kane/patches/2010-09-18/0002-server-Add-mechan... [3/6] ws2_32: Patch to selectively bind to interfaces while still allowing broadcast packets: http://www.compholio.com/wine-kane/patches/2010-09-18/0003-ws2_32-Patch-to-s... [4/6] ws2_32: Ensure select does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0004-ws2_32-Ensure-sel... [5/6] ws2_32: Ensure Async WSARecv does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0005-ws2_32-Ensure-Asy... [6/6] server: Ensure Async ReadFile does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0006-server-Ensure-Asy...
Erich Hoover ehoover(a)mines.edu
Ah sorry, forgot to mention one more thing - fd's are different on the server side and the client side and between processes. So you're locking different semaphores everywhere effectively not locking anything. Mike. On Sat, Sep 18, 2010 at 11:59 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
A few broad comments: - There's enough #ifdef's to make this mostly unreadable. It's hard to think of the cases of no IP_PKTINFO availability, only compile time availability or full availability. Please just write a few wrapper functions for the #ifdef's that return sensible values if IP_PKTINFO is not available and then use them. I understand that in some places (i.e. WS2_send/WS2_recv) are supposed to be #ifdef'y but it may be easier to comment if you minimize the sheer number of combinations. - Pick a place to do the filtering and stick with it. You do some filtering in the server, some filtering in wine-userspace and some calls aren't filtered at all (see blocking recv's). I would suggest forwarding ReadFile/WriteFile to WSARecv/WSASend for sockets and then taking care of it in ws2_32 (we might end up needing this soon anyway for other annoying reasons). Even if your async reads are woken, you should be lenient enough in WS2_recv to return EWOULDBLOCK if no packets matching the interface are available. Since you don't do this now blocking recv's are broken. Also, FYI our *nix sockets are always non-blocking, you don't need MSG_DONTWAIT. - So what if an application likes to bind to an interface AND use IP_PKTINFO? - I don't think this is the right approach. Alexandre's comments from last year still apply. We would have to change the way we do sockets entirely; instead of just passing through to similar functions in *nix we would need to emulate a network stack (which is really what your patches crudely do). For example, now we wouldn't be able to use poll anywhere reliably on sockets. I don't think that's a direction Alexandre wants to go in. I think only one of two things will help us in this case - the kernel folks appease us or wineserver becomes a kernel module and we just do I/O from wineserver. Both of which are on schedule for around the time hell freezes over. - If you really want to get this to work consider writing something to LD_PRELOAD that emulates most of the network syscalls (+ read) with windows-style bind'ing correctly. I'd argue this may be easier and faster than putting the fix it into wine. This may be more user-friendly than having people compile wine themselves.
Mike.
On Sat, Sep 18, 2010 at 10:44 PM, Erich Hoover <ehoover(a)mines.edu> wrote:
I realize it's been some time since I've submitted something for the networking problems with the C&C games, but I believe I've come up with something that resolves the major show-stopping issue that was discussed previously. Here is the original thread, for reference: http://www.winehq.org/pipermail/wine-devel/2009-October/079099.html
Specifically, this set of patches is revised to address the problem of throwing away packets destined for the incorrect interface in a multi-threaded/multi-process environment. The approach used here is to create a global (named) POSIX.1-2001 semaphore for each socket with the form wine-ifacelock-FD , where "FD" is the Unix file descriptor corresponding to the socket at hand. These semaphores are only created for UDP sockets bound to a specific interface and are locked/unlocked by both the Wine user-space and the Wine server.
These patches are against wine-1.3.3 and are split apart according to function. The most revised portions since the original set are 2/6 (server-side creation of semaphore), 3/6 (locking and unlocking), and 6/6 (server-side locking and unlocking). If I could get some feedback on these patches then I would greatly appreciate it, it'd be really nice to finally get this issue resolved. Thanks in advance!
[1/6] ws2_32/tests: Add UDP broadcast tests: http://www.compholio.com/wine-kane/patches/2010-09-18/0001-ws2_32-tests-Add-... [2/6] server: Add mechanism for storing an interface ID with a socket: http://www.compholio.com/wine-kane/patches/2010-09-18/0002-server-Add-mechan... [3/6] ws2_32: Patch to selectively bind to interfaces while still allowing broadcast packets: http://www.compholio.com/wine-kane/patches/2010-09-18/0003-ws2_32-Patch-to-s... [4/6] ws2_32: Ensure select does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0004-ws2_32-Ensure-sel... [5/6] ws2_32: Ensure Async WSARecv does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0005-ws2_32-Ensure-Asy... [6/6] server: Ensure Async ReadFile does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0006-server-Ensure-Asy...
Erich Hoover ehoover(a)mines.edu
The UNIX fd for a Windows socket is not even guaranteed to remain the same throughout a process's lifetime - shuffling fd's between the wineserver and the process changes their value every time. This problem also stops us from SOCKSifying applications running under Wine, because all SOCKSifiers out there assume fd's remain invariant. If you want to uniquely identify a socket despite fd changes, on (at least) Linux, you have to fstat() and consider the device ID/inode values IIRC. On Sun, Sep 19, 2010 at 6:22 AM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
Ah sorry, forgot to mention one more thing - fd's are different on the server side and the client side and between processes. So you're locking different semaphores everywhere effectively not locking anything.
Mike.
On Sat, Sep 18, 2010 at 11:59 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
A few broad comments: - There's enough #ifdef's to make this mostly unreadable. It's hard to think of the cases of no IP_PKTINFO availability, only compile time availability or full availability. Please just write a few wrapper functions for the #ifdef's that return sensible values if IP_PKTINFO is not available and then use them. I understand that in some places (i.e. WS2_send/WS2_recv) are supposed to be #ifdef'y but it may be easier to comment if you minimize the sheer number of combinations. - Pick a place to do the filtering and stick with it. You do some filtering in the server, some filtering in wine-userspace and some calls aren't filtered at all (see blocking recv's). I would suggest forwarding ReadFile/WriteFile to WSARecv/WSASend for sockets and then taking care of it in ws2_32 (we might end up needing this soon anyway for other annoying reasons). Even if your async reads are woken, you should be lenient enough in WS2_recv to return EWOULDBLOCK if no packets matching the interface are available. Since you don't do this now blocking recv's are broken. Also, FYI our *nix sockets are always non-blocking, you don't need MSG_DONTWAIT. - So what if an application likes to bind to an interface AND use IP_PKTINFO? - I don't think this is the right approach. Alexandre's comments from last year still apply. We would have to change the way we do sockets entirely; instead of just passing through to similar functions in *nix we would need to emulate a network stack (which is really what your patches crudely do). For example, now we wouldn't be able to use poll anywhere reliably on sockets. I don't think that's a direction Alexandre wants to go in. I think only one of two things will help us in this case - the kernel folks appease us or wineserver becomes a kernel module and we just do I/O from wineserver. Both of which are on schedule for around the time hell freezes over. - If you really want to get this to work consider writing something to LD_PRELOAD that emulates most of the network syscalls (+ read) with windows-style bind'ing correctly. I'd argue this may be easier and faster than putting the fix it into wine. This may be more user-friendly than having people compile wine themselves.
Mike.
On Sat, Sep 18, 2010 at 10:44 PM, Erich Hoover <ehoover(a)mines.edu> wrote:
I realize it's been some time since I've submitted something for the networking problems with the C&C games, but I believe I've come up with something that resolves the major show-stopping issue that was discussed previously. Here is the original thread, for reference: http://www.winehq.org/pipermail/wine-devel/2009-October/079099.html
Specifically, this set of patches is revised to address the problem of throwing away packets destined for the incorrect interface in a multi-threaded/multi-process environment. The approach used here is to create a global (named) POSIX.1-2001 semaphore for each socket with the form wine-ifacelock-FD , where "FD" is the Unix file descriptor corresponding to the socket at hand. These semaphores are only created for UDP sockets bound to a specific interface and are locked/unlocked by both the Wine user-space and the Wine server.
These patches are against wine-1.3.3 and are split apart according to function. The most revised portions since the original set are 2/6 (server-side creation of semaphore), 3/6 (locking and unlocking), and 6/6 (server-side locking and unlocking). If I could get some feedback on these patches then I would greatly appreciate it, it'd be really nice to finally get this issue resolved. Thanks in advance!
[1/6] ws2_32/tests: Add UDP broadcast tests: http://www.compholio.com/wine-kane/patches/2010-09-18/0001-ws2_32-tests-Add-... [2/6] server: Add mechanism for storing an interface ID with a socket: http://www.compholio.com/wine-kane/patches/2010-09-18/0002-server-Add-mechan... [3/6] ws2_32: Patch to selectively bind to interfaces while still allowing broadcast packets: http://www.compholio.com/wine-kane/patches/2010-09-18/0003-ws2_32-Patch-to-s... [4/6] ws2_32: Ensure select does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0004-ws2_32-Ensure-sel... [5/6] ws2_32: Ensure Async WSARecv does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0005-ws2_32-Ensure-Asy... [6/6] server: Ensure Async ReadFile does not wake up on packets with an interface mismatch: http://www.compholio.com/wine-kane/patches/2010-09-18/0006-server-Ensure-Asy...
Erich Hoover ehoover(a)mines.edu
On Sat, Sep 18, 2010 at 9:59 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
There's enough #ifdef's to make this mostly unreadable. Sorry about this, I was concerned that pushing these out so that changes do not appear directly in the relevant code would make things too obfuscated. Please look at the revised patchset (at bottom).
Pick a place to do the filtering and stick with it. You do some filtering in the server, some filtering in wine-userspace ... The filtering in the server is done to handle throwing packets away from async polls and calls that come through ReadFile.
... and some calls aren't filtered at all (see blocking recv's). The blocking WS_recv() is forwarded to WS2_recvfrom(), which forwards to the "async" WS2_recv()...
I would suggest forwarding ReadFile/WriteFile to WSARecv/WSASend for sockets and then taking care of it in ws2_32 (we might end up needing this soon anyway for other annoying reasons). I'll have to look into this, I am not familiar enough with how Wine handles these operations to make such a change at this time. The ReadFile portion is at least handled through the server-side filtering, though a WriteFile request would go out on all interfaces I'd say that's "not too terrible".
Even if your async reads are woken, you should be lenient enough in WS2_recv to return EWOULDBLOCK if no packets matching the interface are available. Since you don't do this now blocking recv's are broken. This scenario is part of why some of the filtering occurs on the server side, in all honestly, fixing this issue took a long time to work out. It was previously pointed out that returning EWOULDBLOCK in these scenarios would break "broken" applications that do not handle EWOULDBLOCK.
Also, FYI our *nix sockets are always non-blocking, you don't need MSG_DONTWAIT. This is contrary to both my experience and the manual pages for recv() and fcntl()... *nix sockets are non-blocking when O_NONBLOCK is set, which is not necessarily the case as Wine handles both types of sockets.
So what if an application likes to bind to an interface AND use IP_PKTINFO? According to MSDN, while getting the IP_PKTINFO flag is supported, setting it is not.
Alexandre's comments from last year still apply. Unless you're talking about the problem where descriptors are not invariant enough then I don't see how, these changes should make the peek/read combo safe across threads and processes.
For example, now we wouldn't be able to use poll anywhere reliably on sockets. This case should be handled in one of the patches, the revised set (see bottom) should make this more obvious. It looks like I missed locking around some polls, this issue should be corrected in the revised set.
If you really want to get this to work consider writing something to LD_PRELOAD that emulates most of the network syscalls (+ read) with windows-style bind'ing correctly. I'd argue this may be easier and faster than putting the fix it into wine. This may be more user-friendly than having people compile wine themselves. I'm not interested in taking the easy way out here, I would much rather put the appropriate effort into having Wine handle this scenario than making a one-off fix.
On Sat, Sep 18, 2010 at 10:22 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
Ah sorry, forgot to mention one more thing - fd's are different on the server side and the client side and between processes. So you're locking different semaphores everywhere effectively not locking anything. I did not see this in testing, but I understand that it's possible that different systems do not behave the same way. In the revised set (see bottom) I have implemented Damjan's suggestion, which works just as effectively in the testing I've done.
Thank you both for your comments, I hope you find the revised patches below to be more palatable. [1/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0001-ws2_32-tests-Add-... [2/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0002-server-Add-mechan... [3/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0003-ws2_32-Patch-to-s... Erich Hoover ehoover(a)mines.edu
On Sun, Sep 19, 2010 at 1:33 PM, Erich Hoover <ehoover(a)mines.edu> wrote:
On Sat, Sep 18, 2010 at 9:59 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
There's enough #ifdef's to make this mostly unreadable. Sorry about this, I was concerned that pushing these out so that changes do not appear directly in the relevant code would make things too obfuscated. Please look at the revised patchset (at bottom). Thanks that helps a bit. You could still do a few more of those cleanups. Also please don't make a function like WS2_shim_poll; either make it poll-like or call it WS2_shim_select.
Pick a place to do the filtering and stick with it. You do some filtering in the server, some filtering in wine-userspace ... The filtering in the server is done to handle throwing packets away from async polls and calls that come through ReadFile.
That's what I mean - if you need to do filtering for ReadFile - do it in ReadFile (conditionally, there is a way to check if the fd is a socket). Besides the little scheme in the server would only fix async ReadFile requests. If you just called ReadFile with some data pending you would still get it even if it wasn't to the right interface.
... and some calls aren't filtered at all (see blocking recv's). The blocking WS_recv() is forwarded to WS2_recvfrom(), which forwards to the "async" WS2_recv()...
Yes, except do_block would return on any packet received, WS2_recv would return 0 and we would return 0 saying "connection closed" which may not even make sense. We don't really have any tests for UDP which is why you're not seeing any test failures. You fixed it in this set though, but you can still have funky stuff happening (returning 0 from recv because all the packets were filtered out). You really need to return EWOULDBLOCK from WS2_recv for it all to fix itself.
I would suggest forwarding ReadFile/WriteFile to WSARecv/WSASend for sockets and then taking care of it in ws2_32 (we might end up needing this soon anyway for other annoying reasons). I'll have to look into this, I am not familiar enough with how Wine handles these operations to make such a change at this time. The ReadFile portion is at least handled through the server-side filtering, though a WriteFile request would go out on all interfaces I'd say that's "not too terrible".
It's not. Only async requests are. FYI NtReadFile just calls read().
Even if your async reads are woken, you should be lenient enough in WS2_recv to return EWOULDBLOCK if no packets matching the interface are available. Since you don't do this now blocking recv's are broken. This scenario is part of why some of the filtering occurs on the server side, in all honestly, fixing this issue took a long time to work out. It was previously pointed out that returning EWOULDBLOCK in these scenarios would break "broken" applications that do not handle EWOULDBLOCK.
Applications that don't sign up for EWOULDBLOCK (i.e. blocking reads) wouldn't get it since we handle blocking ourselves. WS2_async_recv handles EWOULDBLOCK to return STATUS_PENDING just fine.
Also, FYI our *nix sockets are always non-blocking, you don't need MSG_DONTWAIT. This is contrary to both my experience and the manual pages for recv() and fcntl()... *nix sockets are non-blocking when O_NONBLOCK is set, which is not necessarily the case as Wine handles both types of sockets.
We emulate blocking (see _is_blocking & do_block) ourselves, the actual *nix sockets always have O_NONBLOCK set.
So what if an application likes to bind to an interface AND use IP_PKTINFO? According to MSDN, while getting the IP_PKTINFO flag is supported, setting it is not.
Cool, I didn't realize that. I guess with the way MS designed it IP_PKTINFO would still work correctly.
Alexandre's comments from last year still apply. Unless you're talking about the problem where descriptors are not invariant enough then I don't see how, these changes should make the peek/read combo safe across threads and processes.
This will probably have to be Alexandre's call. I was referring to his first comment mostly.
For example, now we wouldn't be able to use poll anywhere reliably on sockets. This case should be handled in one of the patches, the revised set (see bottom) should make this more obvious. It looks like I missed locking around some polls, this issue should be corrected in the revised set.
That just fixes the problem in ws2_32. We still can't use poll on sockets anywhere else (notably the server). As a consequence FD_READ socket events would now fire for every packet. To fix that you filter out POLLIN when necessary (which is ok), just your reasoning for doing it is a little wrong. WSARecv overlapped requests end up on that queue as well.
If you really want to get this to work consider writing something to LD_PRELOAD that emulates most of the network syscalls (+ read) with windows-style bind'ing correctly. I'd argue this may be easier and faster than putting the fix it into wine. This may be more user-friendly than having people compile wine themselves. I'm not interested in taking the easy way out here, I would much rather put the appropriate effort into having Wine handle this scenario than making a one-off fix.
On Sat, Sep 18, 2010 at 10:22 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
Ah sorry, forgot to mention one more thing - fd's are different on the server side and the client side and between processes. So you're locking different semaphores everywhere effectively not locking anything. I did not see this in testing, but I understand that it's possible that different systems do not behave the same way. In the revised set (see bottom) I have implemented Damjan's suggestion, which works just as effectively in the testing I've done.
It's really unlikely that you wouldn't see it. If you print out fd's on the server side and the wine-userspace side they should be different. We are effectively dup'ing them to get them to wine userspace. (See http://source.winehq.org/source/dlls/ntdll/server.c#L383 )
Thank you both for your comments, I hope you find the revised patches below to be more palatable.
[1/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0001-ws2_32-tests-Add-... [2/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0002-server-Add-mechan... [3/3] http://www.compholio.com/wine-kane/patches/2010-09-19/0003-ws2_32-Patch-to-s...
Erich Hoover ehoover(a)mines.edu
Mike.
On Sun, Sep 19, 2010 at 1:13 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
On Sun, Sep 19, 2010 at 1:33 PM, Erich Hoover <ehoover(a)mines.edu> wrote:
On Sat, Sep 18, 2010 at 9:59 PM, Mike Kaplinskiy <mike.kaplinskiy(a)gmail.com> wrote:
There's enough #ifdef's to make this mostly unreadable. Sorry about this, I was concerned that pushing these out so that changes do not appear directly in the relevant code would make things too obfuscated. Please look at the revised patchset (at bottom). Thanks that helps a bit. You could still do a few more of those cleanups. These defines are no-longer in socket.c at all and they are extremely minimal in the shim ...
Also please don't make a function like WS2_shim_poll; either make it poll-like or call it WS2_shim_select. I apologize here, this is an unfortunate case of over-zealous copy-pasting from the old code - please see the revised patches below.
... you can still have funky stuff happening (returning 0 from recv because all the packets were filtered out). You really need to return EWOULDBLOCK from WS2_recv for it all to fix itself. When these additions filter out a packet after a recv() it then re-calls recv(), so it would return EWOULDBLOCK if there were no un-filtered packets.
You do some filtering in the server, some filtering in wine-userspace ... The filtering in the server is done to handle throwing packets away from async polls and calls that come through ReadFile. That's what I mean - if you need to do filtering for ReadFile - do it in ReadFile (conditionally, there is a way to check if the fd is a socket). Besides the little scheme in the server would only fix async ReadFile requests. If you just called ReadFile with some data pending you would still get it even if it wasn't to the right interface. Please examine the revised patches below, patch 3 redirects ReadFile and WriteFile.
Alexandre's comments from last year still apply. Unless you're talking about the problem where descriptors are not invariant enough then I don't see how, these changes should make the peek/read combo safe across threads and processes. This will probably have to be Alexandre's call. I was referring to his first comment mostly.
For example, now we wouldn't be able to use poll anywhere reliably on sockets. This case should be handled in one of the patches, the revised set (see bottom) should make this more obvious. It looks like I missed locking around some polls, this issue should be corrected in the revised set. That just fixes the problem in ws2_32. We still can't use poll on sockets anywhere else (notably the server). As a consequence FD_READ socket events would now fire for every packet. To fix that you filter out POLLIN when necessary (which is ok), just your reasoning for doing it is a little wrong. WSARecv overlapped requests end up on that queue as well. It seems the comment I put on the server side is from before I had a better understanding of the server-side poll, I apologize and have corrected this issue in the patches below. I believe that I've now covered the different socket poll() cases, is there something else I'm missing here? This issue seemed to be the crux behind previous reservations on implementing this feature (such as Alexandre's first comment), so I would like to make sure that I have satisfactorily resolved this problem.
Thank you for your help on all these issues, it is much appreciated. I believe the patches below will be more to your liking: [1/4] http://www.compholio.com/wine-kane/patches/2010-09-25/0001-ws2_32-tests-Add-... [2/4] http://www.compholio.com/wine-kane/patches/2010-09-25/0002-server-Add-mechan... [3/4] http://www.compholio.com/wine-kane/patches/2010-09-25/0003-ntdll-Pass-ReadFi... [4/4] http://www.compholio.com/wine-kane/patches/2010-09-25/0004-ws2_32-Selectivel... Erich Hoover ehoover(a)mines.edu
participants (3)
-
Damjan Jovanovic -
Erich Hoover -
Mike Kaplinskiy