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

[Bernhard Fischer]

On Fri, Jun 27, 2008 at 03:00:41PM +0200, Ricard Wanderlof wrote:
>
> 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).

I thought that we install everything from include/ on make install_dev,
except the headers that we explicitely wipe afterwards..
>
> 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.

That sounds better since it's just a private header that should not be
installed, yes.

>>> 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 ... ?

I think that it's simple enough to provide a concise version that
has identical functionality.

>>> +
>>> +    __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  

I would just try to set it to 1 in the else { s=socket() block, but it
doesn't matter much if you rewrite __check_pf to just return the
interresting bits.

In the end this addrconfig thing just tries to
static int addrconfig (sa_family_t af)
{
	int tmp, ret, saved_errno = errno;

	tmp = __check_pf();
#ifdef __UCLIBC_HAS_IPV4__
	if (af == AF_INET)
		ret = tmp & SAW_IPv4;
	else
#endif
#ifdef __UCLIBC_HAS_IPV6__
	if (af == AF_INET6)
		ret = tmp & SAW_IPv6;
	else
#endif
	{
		tmp = socket (af, SOCK_DGRAM, 0);
		ret = 1; /* Assume PF_UNIX.  */
		if (tmp < 0) {
			if (errno != EMFILE)
				ret = 0;
		} else
			close (tmp);
	}
	__set_errno (saved_errno);
	return ret;
}
so the __check_pf should either be manually inlined into it or you
should try to inline it via keyword. I don't think it makes much sense
to put __check_pf into a separate file, but that's just a cosmetic
nitpick.

> 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?

Just the places above. Sorry if i was unclear.