Re: [PATCH] getaddrinfo() dest. addr. selection Rule 7 bugfix

[Sridhar Samudrala <sri at us dot ibm dot com>]

On Mon, 2007-03-05 at 17:54 -0800, Ulrich Drepper wrote:
> Sridhar Samudrala wrote:
> > The following patch fixes Rule 7(Prefer native transport) in the destination 
> > address selection process as implemented by getaddrinfo().
> 
> I don't like the patch at all.  Yes, your test for native transport is
> correct but the cost is too high.
> 
> First, we already open a NETLINK_ROUTE socket once, in __check_pf.  The
> code should piggy back on that.  This means reworking how __check_pf is
> used or even works.  The socket has to be created unconditionally while
> the __check_pf body as it exists today should run as before conditional.
>  You'll need to separate opening the socket from the __check_pf code.

Ulrich,

I thought of doing this. Currently __check_pf() is called once for each
getaddrinfo() call before we get the list of destination addresses.
The new RTM_GETROUTE call needs to be done after we get the destination
address list and once for each dest. address.

> 
> Second, the if_name2type code uses yet more syscalls.  These are only
> needed if we ever reach rule 7.  So, delay this work.  And in that
> situation you can use the same socket for both interfaces.
Yes. I agree that if_name2type() should be called only if we reach Rule 7.
Not sure what you meant by 'use the same socket for both interfaces'.
Are you suggesting using the same socket for RTM_GETROUTE and
SIOCGHWADDR calls. Can we do a SIOCGHWADDR ioctl on a netlink socket?

> 
> Third, don't introduce new fields in struct sort_result for yes/no
> information.  You replace in6ai_temporary, which should have given you
> the hint that you should use a flag.  Maybe this is obsoleted by the
> second point above.  In any case, if you remove in6ai_temporary support
> in getaddrinfo.c you also have to remove it where the flag is generated
> (in check_pf.c).
We should remove in6ai_temporary flag, but we cannot replace it with
a flag to store 'native_transport'. The flags in in6addrinfo are for
the local addresses. The native_transport flag is per destination 
address. So i added a field to sort_result. Anyway, We can avoid this
flag if we do if_name2type() only when we hit Rule 7.

> 
> Fourth, your changes violate all kinds of rules of the coding standard.
>  Respect how the rest of the code looks like.  Learn the rules.

I am not fully familiar with the glibc coding standards, but i tried to
follow the syntax of the existing code as much as possible.

Also, i ran into some compile issues when i tried to modify multiple 
files and each build takes 15 minutes. Is it possible to make the builds
faster when we modify a single file?

I think it will be much easier for you to make these changes to fix this
issue and treat this as a bug report with a suggested solution.

Thanks
Sridhar