svn commit: trunk/uClibc: extra/Configs include libc/inet

[Ricard Wanderlof]

On Fri, 27 Jun 2008, Bernhard Fischer wrote:

>> Added: trunk/uClibc/include/ifaddrs.h
>> ===================================================================
>> --- trunk/uClibc/include/ifaddrs.h                             (rev 0)
>> +++ trunk/uClibc/include/ifaddrs.h     2008-06-27 09:08:44 UTC (rev 22531)
>> @@ -0,0 +1,19 @@
>> +#ifndef _IFADDRS_H
>> +#include <libc/inet/ifaddrs.h>
>
> I'm not convinced that the above will work well.

True, it won't work if ifaddrs.h is included outside uclibc (which we 
wouldn't expect it to be).

The bulk of this patch was stuff taken from glibc-2.7 , with the idea of 
minimizing the amount of changes. A similar construct is used there, but 
I'm not sure exactly how it works to be honest.

I propose to concatenate both include files to a single one and put it in 
libc/inet, as the included definitions are only used there.

>> +
>> +extern void __check_pf (bool *seen_ipv4, bool *seen_ipv6)
>
> Perhaps just use unsigned*seen as param.

Or just as a return value as you suggested further down.

>> +#if __UCLIBC_SUPPORT_AI_ADDRCONFIG__
>
> Turn it off and build with -Wundef, then fix it, please.

Sorry, should be #ifdef. I'll fix it as soon as possible.

>> +  {
>> +    /* Get the interface list via getifaddrs.  */
>> +    struct ifaddrs *ifa = NULL;
>> +    struct ifaddrs *runp;
>> +    if (getifaddrs (&ifa) != 0)
>> +    {
>> +      /* We cannot determine what interfaces are available.  Be
>> +      optimistic.  */
>> +      *seen_ipv4 = true;
>
> No. Doesn't take __UCLIBC_HAS_IPV4__ into account.

Correct, will fix this too (and the other two places in the same file).

>> +#if __UCLIBC_HAS_IPV6__
>
> -Wundef

Same here.

>> static int addrconfig (sa_family_t af)
>> {
>>     int s;
>>     int ret;
>>     int saved_errno = errno;
>> -    s = socket(af, SOCK_DGRAM, 0);
>> -    if (s < 0)
>> -      ret = (errno == EMFILE) ? 1 : 0;
>> +    bool seen_ipv4;
>> +    bool seen_ipv6;
>
> yea, no. one variable and mask them. Or, even better
> unsigned __check_pf(void)

Again, the reason this function looks like it does is because it was 
snitched from glibc. That is probably not a strong enough reason not to 
change it though ... ?

>> +
>> +    __check_pf(&seen_ipv4, &seen_ipv6);
>> +    if (af == AF_INET)
>> +      ret = (int)seen_ipv4;
>> +    else if (af == AF_INET6)
>> +      ret = (int)seen_ipv6;
>>     else
>>     {
>> -      close(s);
>> -      ret = 1;
>> +      s = socket(af, SOCK_DGRAM, 0);
>> +      if (s < 0)
>> +          ret = (errno == EMFILE) ? 1 : 0;
>> +      else
>> +      {
>> +          close(s);
>> +          ret = 1;
>> +      }
>
> Why don't you just ret=1 and adjust ret later on accordingly?

See above.

The diff looks confusing. In plain, the function looks like this:

static int addrconfig (sa_family_t af)
{
     int s;
     int ret;
     int saved_errno = errno;
     bool seen_ipv4;
     bool seen_ipv6;

     __check_pf(&seen_ipv4, &seen_ipv6);
     if (af == AF_INET)
 	ret = (int)seen_ipv4;
     else if (af == AF_INET6)
 	ret = (int)seen_ipv6;
     else
     {
 	s = socket(af, SOCK_DGRAM, 0);
 	if (s < 0)
 	    ret = (errno == EMFILE) ? 1 : 0;
 	else
 	{
 	    close(s);
 	    ret = 1;
 	}
     }
     __set_errno (saved_errno);
     return ret;
}

I could change this to

static int addrconfig (sa_family_t af)
{
     int s;
     int ret = 1;
     int saved_errno = errno;
     bool seen_ipv4;
     bool seen_ipv6;

     __check_pf(&seen_ipv4, &seen_ipv6);
     if (af == AF_INET)
 	ret = (int)seen_ipv4;
     else if (af == AF_INET6)
 	ret = (int)seen_ipv6;
     else
     {
 	s = socket(af, SOCK_DGRAM, 0);
 	if (s < 0) {
 	    if (errno != EMFILE)
 	        ret = 0;
 	}
 	else
 	    close(s);
     }
     __set_errno (saved_errno);
     return ret;
}

but the resulting code is a couple of bytes larger, and not as easy to 
follow.

> And please do not introduce warnings about using undefined preprocessor
> tokens and also keep in mind that we ultimately want a libc that can be
> configured as IPv6-only. Thanks.

I assume you are referring to your comments above, or were there more 
places that you were thinking of?

Thanks for your comments.

/Ricard
--
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30