Re: ws2_32: Use the routing table information for gethostbyname('self') if the magic IP would otherwise be returned.

[Vitaliy Margolen]

Erich Hoover wrote:
> Real Name:
>     Erich Hoover
> Description:
>     If an application calls gethostbyname on it's own hostname then it
> expects an IP to be returned for each interface, in order of the
> routing priority (see MS Knowledge Base article #160215).  The
> attached patch first checks to see if the gethostbyname request
> resolved to a loopback address (which currently gets replaced by the
> magic IP).  If the magic IP would have been returned then it uses the
> routing table to build the list of IP addresses and properly sorts
> those addresses from highest routing priority to lowest routing
> priority (lowest metric to highest metric).
> Changelog:
>     ws2_32: Use the routing table information for
> gethostbyname('self') if the magic IP would otherwise be returned.

Thanks for the patch, however there are few issues with it.

Why do you return a list of IP addresses from WS2_get_local_ips if you need
only one? Just get the one with the lowest metric and return it.

I think it would be better
> - adapters = HeapAlloc(GetProcessHeap(),0,adap_size);
> - routes = HeapAlloc(GetProcessHeap(),0,route_size);
> - route_addrs = HeapAlloc(GetProcessHeap(),0,0); /* HeapReAlloc doesn't work on NULL */

Here and everywhere else please put space before comas to follow the file's
formatting.
> - if (route_addrs != NULL)
> -
>    HeapFree(GetProcessHeap(), 0, route_addrs);
> - if (adapters != NULL)
> -
>    HeapFree(GetProcessHeap(), 0, adapters);
> - if (routes != NULL)
> -
>    HeapFree(GetProcessHeap(), 0, routes);

You do not need to check pointers for NULL before freeing them.

Vitaliy.

ws2_32: Use the routing table information for gethostbyname('self') if the magic IP would otherwise be returned.

[Erich Hoover]

On Sat, Oct 10, 2009 at 2:52 PM, Vitaliy Margolen
wrote:
> ...
> Why do you return a list of IP addresses from WS2_get_local_ips if you need
> only one? Just get the one with the lowest metric and return it.
> ...

Well, most applications only check for the first returned IP (the one with
the highest priority, or the lowest metric).  However, from reading the
knowledge base article the purpose of this "feature" of the routine is to
return them all.

...
> Here and everywhere else please put space before comas to follow the file's
> formatting.

Sorry, I copied what I saw for HeapAlloc in the WS_gethostbyname routine
(nearby).  Would you like a patch for updates to the spacing there as well?

...
> You do not need to check pointers for NULL before freeing them.

Sorry, when I see "the behavior is undefined" in documentation I tend to shy
away doing something.

Thanks for the feedback!

[e-mail]
Erich Hoover

ws2_32: Use the routing table information for gethostbyname('self') if the magic IP would otherwise be returned.

[Kai Blin]

On Saturday 10 October 2009 22:52:48 Vitaliy Margolen wrote:
> > Changelog:
> >     ws2_32: Use the routing table information for
> > gethostbyname('self') if the magic IP would otherwise be returned.
>
> Thanks for the patch, however there are few issues with it.
>
> Why do you return a list of IP addresses from WS2_get_local_ips if you need
> only one? Just get the one with the lowest metric and return it.
>
> I think it would be better

That's not what Windows does, though. Windows returns the full list.

Just my 2 cents,
Kai

ws2_32: Use the routing table information for gethostbyname('self') if the magic IP would otherwise be returned.

[Vitaliy Margolen]

Kai Blin wrote:
> On Saturday 10 October 2009 22:52:48 Vitaliy Margolen wrote:
> > > Changelog:
> > >     ws2_32: Use the routing table information for
> > > gethostbyname('self') if the magic IP would otherwise be returned.
> >
> > Thanks for the patch, however there are few issues with it.
> >
> > Why do you return a list of IP addresses from WS2_get_local_ips if you need
> > only one? Just get the one with the lowest metric and return it.
> >
> > I think it would be better
>
> That's not what Windows does, though. Windows returns the full list.

Got you, then what the patch does is still can be improved.

Since patch using WS2_get_local_ips() in one place only return something you
can just pass directly to the app, not something that needs duplicating and
then freeing.

Vitaliy.

ws2_32: Use the routing table information for gethostbyname('self') if the magic IP would otherwise be returned.

[Erich Hoover]

On Sun, Oct 11, 2009 at 6:01 PM, Vitaliy Margolen
wrote:
> Kai Blin wrote:
> > On Saturday 10 October 2009 22:52:48 Vitaliy Margolen wrote:
> > > > Changelog:
> > > >     ws2_32: Use the routing table information for
> > > > gethostbyname('self') if the magic IP would otherwise be returned.
> > >
> > > Thanks for the patch, however there are few issues with it.
> > >
> > > Why do you return a list of IP addresses from WS2_get_local_ips if you
>
> need
> > > only one? Just get the one with the lowest metric and return it.
> > >
> > > I think it would be better
> >
> > That's not what Windows does, though. Windows returns the full list.
>
> Got you, then what the patch does is still can be improved.
>
> Since patch using WS2_get_local_ips() in one place only return something
> you
> can just pass directly to the app, not something that needs duplicating and
> then freeing.

When I was first testing that's actually what I coded this routine to do,
but the documentation indicates that the hostent returned by gethostbyname
is not supposed to be freed by the application.  I assumed that part of the
purpose of the WS_dup_he was to unify returning hostents so that (at some
point down the line) the old memory allocated for them could be freed when
there's a new call.

Erich Hoover
[e-mail]