Why ISC developing wheel again?

[Mark Andrews]

> 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 imple
> me=
> > > nt all standard libc functions. Quick example - nsupdate.c source contain
> s =
> > > isc_string_printf function. But BIND source also contains isc_print_snpri
> nt=
> > > 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

	isc_string_printf and isc_string_printf_truncate are NOT
	the private implenentations of sprintf.  isc_print_sprintf()
	is the private implementation of sprintf()
 
> > 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 fun
> ction in nsupdate.c directly?

	isc_string_printf != isc_print_sprintf

	This is isc_string_printf().  It is NOT a replacement for missing
	C99 functionality.

	[It also need the terminating \0 to be added in the error handling]

isc_result_t
isc_string_printf(char *target, size_t size, const char *format, ...) {
        va_list args;
        size_t n;

        REQUIRE(size > 0U);

        va_start(args, format);
        n = vsnprintf(target, size, format, args);
        va_end(args);

        if (n >= size) {
                memset(target, ISC_STRING_MAGIC, size);
                return (ISC_R_NOSPACE);
        }

        ENSURE(strlen(target) < size);

        return (ISC_R_SUCCESS);
}


> You should use snprintf instead because you hav
> e #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 me
> rged to one. Yes, I can look like pedantic person but I only want clean code 
> :)

	You have clean code.  We don't call isc_print_snprintf anywhere.

% grep -r isc_print_snprintf lib bin
lib/isc/print.c:isc_print_snprintf(char *str, size_t size, const char *format, ...) {
lib/isc/include/isc/print.h:isc_print_snprintf(char *str, size_t size, const char *format, ...)
lib/isc/include/isc/print.h:#define snprintf isc_print_snprintf
% 

	On most modern platforms lib/isc/print.c is not even compiled.
	If we were to call isc_print_snprintf() rather than snprintf()
	most (all?) of our test platforms would complain.
 
> > > Why we have those duplicated functions? I know that you =
> > > want run BIND on all systems. But implementing all infrastructure directl
> y =
> > > 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 li
> bra=
> > > 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 lib
> rary.
> 
> Adam
> 
-- 
Mark Andrews, ISC
1 Seymour St., Dundas Valley, NSW 2117, Australia
PHONE: +61 2 9871 4742                 INTERNET: Mark_Andrews at isc.org