On Wed, Nov 25, 2015 at 5:57 AM, Jamie Taylor Jamie.Taylor@pobox.com wrote:
On Tue, Nov 24, 2015 at 08:30:13PM +0800, Bruno Jesus wrote:
Hi, Jamie. Thanks for your effort, as you may have observed in the patches list your patch is marked as deferred because we are in code freeze for Wine 1.8 release. This code was touched a few times in attempts to fix this same issue you are addressing so maybe this time it will be the definitive fix =)
Hello, Bruno. Thanks for your review.
I think that a true definitive fix would include removing the magic loopback address completely, as it's obviously a hack (and not a value that would ever be returned by Windows). That seems like a much more invasive change.
I agree to that, the magic IP is used when the interface does not have an IP but has to be returned, that is why it was invented I believe.
I have a similar (not so robust) old patch that I used on FreeBSD (and also works in Linux), could you try it in OSX just out of curiosity, please? It has printf to help showing the results so a make test in the ws2_32/tests folder is enough to check if it works.
Yes, that patch works for me. The main difference is that it includes the magic loopback address in the returned list if there are any routes to a loopback interface in the forwarding table (actually, one copy of the magic loopback address per loopback interface, I think... but there's generally only one loopback interface). And it doesn't return all of the addresses from an interface (although I've never actually seen one have more than one address - but the data structure supports it).
I'll make some manual tests related about multiple IP addresses in the same interface and to check if the loopback address should really be returned.
Is the resulting list of addresses supposed to contain loopback addresses? (I.e., does it on Windows? I don't have a Windows machine on which to test.) If so, then the patch that I submitted is deficient in that regard, as it will never return addresses from loopback interfaces. (Of course, the existing implementation and your patch will both return the magic loopback address rather than any addresses actually assigned to the interface, but that's a different problem.)
In any case, either patch will solve the specific problem that I was trying to solve (getting the correct IP address displayed when hosting a Diablo 2 game), so in that respect I don't care which one makes it into a release. (In fact, now that I have a Wineskin with a custom wine build that works for me, I don't care if it ever makes it into a release - but I assume other people who don't have the ability to fix it themselves would like it, which is why I submitted a patch in the first place.)
I believe we need more tests to understand this problem better but your patch is better than mine because it simplifies the logic and removes the realloc so I would prefer something like it but we still need to test the loopback thing.
About your patch I just would like to say that you should follow the surrounding code style, so the { goes in the next line. I tested it on Linux and it works for Heroes of Might & Magic 2 (https://bugs.winehq.org/show_bug.cgi?id=22819).
I was following the style used by 4 of the 7 opening braces in WS_gethostbyname (for which WS_get_local_ips is a helper, and which appears immediately following WS_get_local_ips in socket.c). Perhaps someone should change the 10% of the open braces in this file that do not appear alone on their lines if they are all meant to use a consistent style?
Slowly we are changing into that direction but we don't do style only changes, so only when the code is really touched we will change that.
And speaking of style, I suppose the indentation should be 4 spaces rather than whatever default my editor used?
Yes, in this DLL we use four spaces I believe most of the time. There may be some tabs still around.
Bruno