Why ISC developing wheel again?

[Adam Tkac]

On Fri, Oct 05, 2007 at 01:00:13AM +1000, Mark Andrews wrote:
> 
> > Hi all,
> > 
> > I've checkouted 9.5.0a7 from CVS and I'm really baffled how ISC try impleme=
> > nt all standard libc functions. Quick example - nsupdate.c source contains =
> > isc_string_printf function. But BIND source also contains isc_print_snprint=
> > f function. And C99 standard defines snprintf() function. All those three f=
> > unctions are same.
> 
> 	Have you actually read the code?

yes

> 
> 	isc_print_snprintf() is there only for those platforms where
> 	configure cannot find a working (C 99) snprintf().  We do
> 	have people that compile named on such platforms.  The code
> 	to do this was added circa Jun '03.  If we find a working
> 	sprintf() we use it.  Similarly for vsnprintf().

try compile nsupdate and then

$nm nsupdate |grep isc_string_printf
08140d40 T isc_string_printf
08140c80 T isc_string_printf_truncate

> 
> AC_MSG_CHECKING(sprintf)
> AC_TRY_COMPILE([
> #include <stdio.h>
> ],
> [ char buf[2]; return(*sprintf(buf,"x"));],
> [
> ISC_PRINT_OBJS="print.$O"
> ISC_PRINT_SRCS="print.c"
> ISC_PLATFORM_NEEDSPRINTF="#define ISC_PLATFORM_NEEDSPRINTF"
> LWRES_PLATFORM_NEEDSPRINTF="#define LWRES_PLATFORM_NEEDSPRINTF"
> ],
> [ISC_PLATFORM_NEEDSPRINTF="#undef ISC_PLATFORM_NEEDSPRINTF"
>  LWRES_PLATFORM_NEEDSPRINTF="#undef LWRES_PLATFORM_NEEDSPRINTF"]
> )
> 
> 	isc_string_printf() and isc_string_printf_truncate() both
> 	call vsnprintf() and enforce additional semantics, i.e. no
> 	accidental truncation of the output.

Yes, this looks fine. But I don't know why you're using isc_string_printf function in nsupdate.c directly? You should use snprintf instead because you have #define snprintf isc_print_snprintf in lib/isc/include/isc/print.h. I also don't know why functions isc_print_snprintf and isc_string_printf won't be merged to one. Yes, I can look like pedantic person but I only want clean code :)

> 
> > Why we have those duplicated functions? I know that you =
> > want run BIND on all systems. But implementing all infrastructure directly =
> > in BIND is very very bad.
> 
> 	We don't.  If it's not needed we don't compile it.
> 
> > If I compare that backward compatibility with rec=
> > ent discussion about EDNS0 and edns option (you said EDNS0 is 8 years old, =
> > all should implement it) how old is C99 standard?  Why you don't trust libra=
> > ry developers and you try implement all infrastructure yourself?
> 
> 	We do trust them for the most part.  It's only after testing
> 	the library's functionality, and find it wanting, that we
> 	implement it ourselves.
> 
> 	I was involved in specing inet_pton() and inet_ntop() so I
> 	know NetBSD's implementation is wrong.  I deliberately
> 	didn't want all the possible inputs to inet_addr() to be
> 	accepted.  Too many people were %03d to print out the octets
> 	of IP addresses then claiming them to be valid IP addresses.
> 	By restricting the input space the ambiguity disappeared.
> 	Then there were libraries that half implemented inet_pton().

I don't have anything against this approach but I still prefer fix NetBSD library.

Adam

> 
> # On NetBSD 1.4.2 and maybe others, inet_pton() incorrectly accepts
> # addresses with less than four octets, like "1.2.3".  Also leading
> # zeros should also be rejected.
> 
> AC_MSG_CHECKING([for working inet_pton with IPv6 support])
> AC_TRY_RUN([
> #include <sys/types.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <arpa/inet.h>
> main() { char a[16]; return (inet_pton(AF_INET, "1.2.3", a) == 1 ? 1 :
>                              inet_pton(AF_INET, "1.2.3.04", a) == 1 ? 1 :
>                              (inet_pton(AF_INET6, "::1.2.3.4", a) != 1)); }],
>         [AC_MSG_RESULT(yes)
>         ISC_PLATFORM_NEEDPTON="#undef ISC_PLATFORM_NEEDPTON"],
>         [AC_MSG_RESULT(no)
>         ISC_EXTRA_OBJS="$ISC_EXTRA_OBJS inet_pton.$O"
>         ISC_EXTRA_SRCS="$ISC_EXTRA_SRCS inet_pton.c"
>         ISC_PLATFORM_NEEDPTON="#define ISC_PLATFORM_NEEDPTON 1"],
>         [AC_MSG_RESULT(assuming target platform has working inet_pton)
>         ISC_PLATFORM_NEEDPTON="#undef ISC_PLATFORM_NEEDPTON"],
>         [AC_MSG_RESULT(assuming inet_pton needed)
>         ISC_EXTRA_OBJS="$ISC_EXTRA_OBJS inet_pton.$O"
>         ISC_EXTRA_SRCS="$ISC_EXTRA_SRCS inet_pton.c"
>         ISC_PLATFORM_NEEDPTON="#define ISC_PLATFORM_NEEDPTON 1"],
>         [AC_MSG_RESULT(assuming target platform has working inet_pton)
>         ISC_PLATFORM_NEEDPTON="#undef ISC_PLATFORM_NEEDPTON"])
> 
> > nsupdate c=
> > hangelog says "Cleanup nsupdates GSS-TSIG support" - but I think better ter=
> > m will be "Create more confusions in nsupdate". Please think again about th=
> > at nasty backward compatibility. And if you decide you need it better shoul=
> > d be use configure defined macros like #ifndef HAVE_FUNCTION use_isc_one() =
> > #endif. If I exaggerate it one fine day you decide kernel has many bugs so =
> > why don't implement it in BIND? :)
> > 
> > Adam
> -- 
> Mark Andrews, ISC
> 1 Seymour St., Dundas Valley, NSW 2117, Australia
> PHONE: +61 2 9871 4742                 INTERNET: Mark_Andrews at isc.org