Requesting comments on patchset to fix Bug 7929 (C&C 3 network does not work)
I've been a bit busy lately, but I sent in a patchset at the end of September to fix the C&C3 LAN networking bug. If I could get some feedback on these patches then I would greatly appreciate it: http://www.winehq.org/pipermail/wine-patches/2009-September/079193.html[1/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079195.html[2/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079194.html[3/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079198.html[4/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079196.html[5/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079197.html[6/6] Erich Hoover ehoover(a)mines.edu
Hi Eric, it seems to me that if this is the best we can do, we're fixing it at the wrong layer. Surely putting the fix in the Linux kernel would be much smaller in code size, and higher performing, as we wouldn't have to filter packets in user space. --Juan
On Fri, Oct 9, 2009 at 5:15 PM, Juan Lang <juan.lang(a)gmail.com> wrote:
Hi Eric,
it seems to me that if this is the best we can do, we're fixing it at the wrong layer. Surely putting the fix in the Linux kernel would be much smaller in code size, and higher performing, as we wouldn't have to filter packets in user space. --Juan
I put this patchset together after reviewing several years of conversations people have had with the kernel developers. A simple summary of what the devs have said is that the functions in the kernel that provide the ability to do this "easily" require root privileges for very good reasons (essentialy, the easy ways of approaching this problem allow overriding certain permissions mechanisms). Their suggestion was to filter using IP_PKTINFO (which is what the patches do): http://www.mail-archive.com/linux-net(a)vger.kernel.org/msg01341.html The maintainer has pretty much "put his foot down" on the matter (several times actually, here's a nicer one): http://www.mail-archive.com/linux-net(a)vger.kernel.org/msg01306.html This is rather embarrasing, but apparently I left server/protocol.def out of the patchset. I could have sworn I tested these patches on a clean git, but apparently I made a mistake. Is there any chance that this mistake is the reason for the rejection? The additional code in these patches is only utilized (sans a call to getsockopt) on UDP broadcast sockets that have been bound to a specific interface. According to the kernel devs, this behavior is what IP_PKTINFO is meant to do and that they have no intention of adding an additional feature that does exactly the same thing. Erich Hoover ehoover(a)mines.edu
This is rather embarrasing, but apparently I left server/protocol.def out of the patchset. I could have sworn I tested these patches on a clean git, but apparently I made a mistake. Is there any chance that this mistake is the reason for the rejection?
It very well could be, but you'll have better luck getting the opinion of The Man. If you ask nicely he might chime in, or catch him on irc. --Juan
Erich Hoover <ehoover(a)mines.edu> writes:
The maintainer has pretty much "put his foot down" on the matter (several times actually, here's a nicer one): http://www.mail-archive.com/linux-net(a)vger.kernel.org/msg01306.html
This is rather embarrasing, but apparently I left server/protocol.def out of the patchset. I could have sworn I tested these patches on a clean git, but apparently I made a mistake. Is there any chance that this mistake is the reason for the rejection? The additional code in these patches is only utilized (sans a call to getsockopt) on UDP broadcast sockets that have been bound to a specific interface. According to the kernel devs, this behavior is what IP_PKTINFO is meant to do and that they have no intention of adding an additional feature that does exactly the same thing.
I don't think you can do that reliably in user space, at least not in the Wine architecture. You'd have to put all the network I/O in the wineserver, and performance would suck. The kernel devs objection would work if it could be done directly inside the app, but I don't see a way to do that cleanly inside the Wine network layer. -- Alexandre Julliard julliard(a)winehq.org
Maybe I'm missing something, but the only case I found that required extra wineserver work was for handling asynchronous ReadFile requests (since these requests don't go through the ws2_32 path, see patch 6). Using ReadFile like that is technically something you're not supposed to do, but we all know that means that someone's done it anyway. For everything else (send, recv, select) the proposed technique just requires a check to see if IP_PKTINFO is enabled so it can throw out the packet if it doesn't match the socket's interface. The kernel developers have made it clear that they're not going to do anything to change the situation (and even if they did, it wouldn't work under BSD or Mac OS X), so it seems to me like the change has to be in Wine. It would also be possible to override the appropriate calls by replacing the different dynamically loaded symbols, though doing so seems like an awkward approach since Wine is already meant to be a compatibility layer. I would really like to see this issue get resolved, so I'm willing to put whatever work into fixing this that's necessary to get it done right. Erich Hoover ehoover(a)mines.edu On Sat, Oct 10, 2009 at 2:31 AM, Alexandre Julliard <julliard(a)winehq.org>wrote:
Erich Hoover <ehoover(a)mines.edu> writes:
The maintainer has pretty much "put his foot down" on the matter (several times actually, here's a nicer one): http://www.mail-archive.com/linux-net(a)vger.kernel.org/msg01306.html
This is rather embarrasing, but apparently I left server/protocol.def out of the patchset. I could have sworn I tested these patches on a clean git, but apparently I made a mistake. Is there any chance that this mistake is the reason for the rejection? The additional code in these patches is only utilized (sans a call to getsockopt) on UDP broadcast sockets that have been bound to a specific interface. According to the kernel devs, this behavior is what IP_PKTINFO is meant to do and that they have no intention of adding an additional feature that does exactly the same thing.
I don't think you can do that reliably in user space, at least not in the Wine architecture. You'd have to put all the network I/O in the wineserver, and performance would suck. The kernel devs objection would work if it could be done directly inside the app, but I don't see a way to do that cleanly inside the Wine network layer.
-- Alexandre Julliard julliard(a)winehq.org
Erich Hoover <ehoover(a)mines.edu> writes:
Maybe I'm missing something, but the only case I found that required extra wineserver work was for handling asynchronous ReadFile requests (since these requests don't go through the ws2_32 path, see patch 6). Using ReadFile like that is technically something you're not supposed to do, but we all know that means that someone's done it anyway. For everything else (send, recv, select) the proposed technique just requires a check to see if IP_PKTINFO is enabled so it can throw out the packet if it doesn't match the socket's interface.
The main problem is you can't throw out packets reliably in a multithread/multiprocess environment. I know this is UDP, but still I don't think it's acceptable to risk throwing out valid packets. -- Alexandre Julliard julliard(a)winehq.org
On Saturday 10 October 2009 01:15:17 Juan Lang wrote:
Hi Eric,
it seems to me that if this is the best we can do, we're fixing it at the wrong layer. Surely putting the fix in the Linux kernel would be much smaller in code size, and higher performing, as we wouldn't have to filter packets in user space.
Been there, got wristslapped. The network subsystem maintainer of the Linux kernel is pretty clear that he won't add quirky windows behaviour to the Linux network stack just so Wine can get some apps to work. Erich's work seems to be the first real solution to a problem that affects a couple of games with developers who didn't get networking right. I'm pretty sure this only works by accident on windows as well, it'll break on dual-homed windows hosts just the way it breaks on Wine. Cheers, Kai -- Kai Blin WorldForge developer http://www.worldforge.org/ Wine developer http://wiki.winehq.org/KaiBlin Samba team member http://www.samba.org/samba/team/ -- Will code for cotton.
2009/10/9 Erich Hoover <ehoover(a)mines.edu>:
I've been a bit busy lately, but I sent in a patchset at the end of September to fix the C&C3 LAN networking bug. If I could get some feedback on these patches then I would greatly appreciate it:
http://www.winehq.org/pipermail/wine-patches/2009-September/079193.html [1/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079195.html [2/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079194.html [3/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079198.html [4/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079196.html [5/6] http://www.winehq.org/pipermail/wine-patches/2009-September/079197.html [6/6]
You could at least work at getting the tests into wine as a first step, even if your fix isn't accepted. It would also be a good idea to add tests for this behaviour as well. This would then lay the groundwork for a fix in the future. Is it worth getting these into the winehacks tree? - Reece
participants (5)
-
Alexandre Julliard -
Erich Hoover -
Juan Lang -
Kai Blin -
Reece Dunn