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. _______________________________________________ uClibc mailing list uClibc@uclibc.org http://busybox.net/cgi-bin/mailman/listinfo/uclibc