Re: RFC: libkern version of inet_ntoa_r

On Sat, Jul 28, 2012 at 10:14:10PM +0000, Bjoern A. Zeeb wrote:
> On Wed, 25 Jul 2012, Luigi Rizzo wrote:
> 
> >During some ipfw/dummynet cleanup i noticed that the libkern version of
> >inet_ntoa_r() is missing the buffer size argument that is present in
> >the libc counterpart.
> >
> >Any objection if i fix it ?
> 
> And why exactly would you need it?  What does libc do with it?  Render
> partial IPv4 addresses?

Thanks for raising the issue because things are actually simpler
than i thought.


In general, having a function with same name and different
arguments/behaviour in the kernel and userspace is annoying and
should be avoided/fixed if possible.

We have to live with malloc/free as they are too widely used to suggest
a change, but this inet_ntoa_r() is very seldom used.
In fact, the libc version is never used in HEAD, stable/9 and stable/8 
and only the libkern version has a small number of clients (see below).

Given that libkern has inet_ntop, with the same arguments of the
userspace version, we'd be much better off with the following
course of action:

1. replace calls to inet_ntoa_r() with inet_ntop()
   This needs to be done only in the kernel, because the libc version
   is never used at least in the source tree (maybe some port does).

2. nuke inet_ntoa_r() from libkern

3. nuke inet_ntoa_r() from libc

The discussion on the cost of the extra argument is not very
relevant here, because the function is intrinsically slow and
called on slow paths (involves an snprintf, and likely some
device I/O downstream).

cheers
luigi


# grep -r	net_ntoa_r . | grep -Ev pristine
./include/arpa/inet.h:#define	inet_ntoa_r		__inet_ntoa_r
./include/arpa/inet.h:char		*inet_ntoa_r(struct in_addr, char *buf, socklen_t size);
./lib/libc/inet/Symbol.map:	__inet_ntoa_r;
./lib/libc/inet/Symbol.map:	inet_ntoa_r;
./lib/libc/inet/inet_ntoa.c:inet_ntoa_r(struct in_addr in, char *buf, socklen_t size)
./lib/libc/inet/inet_ntoa.c:__weak_reference(__inet_ntoa_r, inet_ntoa_r);
./lib/libc/net/Makefile.inc:	inet.3 inet_network.3 inet.3 inet_ntoa.3 inet.3 inet_ntoa_r.3\
./lib/libc/net/inet.3:.Nm inet_ntoa_r ,
./lib/libc/net/inet.3:.Fo inet_ntoa_r
./lib/libc/net/inet.3:.Fn inet_ntoa_r
./sys/libkern/inet_ntoa.c:inet_ntoa_r(struct in_addr ina, char *buf)
./sys/net/flowtable.c:		inet_ntoa_r(ssin->sin_addr, saddr);
./sys/net/flowtable.c:		inet_ntoa_r(dsin->sin_addr, daddr);
./sys/net/flowtable.c:		inet_ntoa_r(*(struct in_addr *) &dsin->sin_addr, daddr);
./sys/net/flowtable.c:	inet_ntoa_r(*(struct in_addr *) &hashkey[2], daddr);
./sys/net/flowtable.c:		inet_ntoa_r(*(struct in_addr *) &hashkey[1], saddr);		
./sys/net/if_llatbl.c:		inet_ntoa_r(sin->sin_addr, l3s);
./sys/netinet/ipfw/ip_fw_dynamic.c:		inet_ntoa_r(da, src);
./sys/netinet/ipfw/ip_fw_dynamic.c:		inet_ntoa_r(da, dst);
./sys/netinet/ipfw/ip_fw_dynamic.c:			inet_ntoa_r(da, src);
./sys/netinet/ipfw/ip_fw_dynamic.c:			inet_ntoa_r(da, dst);
./sys/netinet/ipfw/ip_fw_dynamic.c:		inet_ntoa_r(da, src);
./sys/netinet/ipfw/ip_fw_dynamic.c:		inet_ntoa_r(da, dst);
./sys/netinet/ipfw/ip_fw_dynamic.c:						inet_ntoa_r(da, src);
./sys/netinet/ipfw/ip_fw_dynamic.c:						inet_ntoa_r(da, dst);
./sys/netinet/ipfw/ip_fw_log.c:			inet_ntoa_r(ip->ip_src, src);
./sys/netinet/ipfw/ip_fw_log.c:			inet_ntoa_r(ip->ip_dst, dst);
./sys/netinet/in.h:char	*inet_ntoa_r(struct in_addr ina, char *buf); /* in libkern */
./sys/netinet/in_pcb.c:		inet_ntoa_r(inc->inc_laddr, laddr_str);
./sys/netinet/in_pcb.c:		inet_ntoa_r(inc->inc_faddr, faddr_str);
./sys/netinet/tcp_subr.c:		inet_ntoa_r(inc->inc_faddr, sp);
./sys/netinet/tcp_subr.c:		inet_ntoa_r(inc->inc_laddr, sp);
./sys/netinet/tcp_subr.c:		inet_ntoa_r(ip->ip_src, sp);
./sys/netinet/tcp_subr.c:		inet_ntoa_r(ip->ip_dst, sp);
>  
`
I need it because i would like to compile parts of the kernel in userspace,
and having a kernel function with the same name and different arguments
from of a libc function is annoying.
I can accept that

> -- 
> Bjoern A. Zeeb                                 You have to have visions!
>          Stop bit received. Insert coin for new address family.