ws2_32: Update WS_get_local_ips to not return a bogus IP address.

[Bruno Jesus]

On Wed, Nov 25, 2015 at 5:57 AM, Jamie Taylor <Jamie.Taylor at 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