Re: squid-3.HEAD IPAddress leak

On Sat, 2008-02-09 at 01:10 +1300, Amos Jeffries wrote:
> >> struct addrinfo
> >> {
> >> int ai_family;
> >> int ai_socktype;
> >> int ai_protocol;
> >> struct sockaddr *ai_addr; // pointer to sockaddr_storage/*_in/*_in6
> >> int ai_addrlen;
> >> char *ai_canonname; // we never new/free this ourselves in squid.
> >> struct addrinfo *ai_next; // Pointer to next in list.
> >> };
> >
> > Can you replace IPAddress data members with the above, except not use
> > any pointers and forget about ai_next and ai_canonname? I think doing so
> > will eliminate temporary allocations and other things that look rather
> > scary to both code quality and performance folks.
>
> In the cases where addrinfo* is going into read-only usage yes.

My understanding is that you will not need addrinfo in most cases, not
for read-only usage, not for read-write usage. In fact, I do not see a
single line in comm.cc that would pass addrinfo to some system call! I
am sure I missed something, but clearly the majority of addrinfo uses is
just to store basic information like address family or protocol.

We create a complex structure, store basic information in there, use it,
and destroy. When we use that information, we pass it to most system
calls piece-by-piece, not as addrinfo structure! This tells me that we
do not need this complex temporary storage. We do not use that
complexity, but are paying a price both in terms of performance and in
terms of code quality.

Could you please make IPAddress store sockaddr_storage address (and
whatever other basic pieces of information you think we must store or
should cache for performance reasons) and remove (Get|Init|
Free)AddrInfo() calls from Squid code where addrinfo does not have to be
passed to system calls?

IMO, doing the above will improve the overall code quality a lot and
will address performance concerns raised on this thread.

Thank you,

Alex.
P.S. It is obviously unfortunate that I am making these comments after
IPAddress changes were committed to HEAD. To be honest, I just could not
imagine that supporting IPv6 would result in this kind of code changes.
I expected low-level IPv4 address storage to be replaced with IPAddress
class, leaving core code mostly intact. I do apologize for not reviewing
such a significant contribution earlier.
Received on Fri Feb 08 2008 - 08:45:52 MST