Request for comment for async resolution of DNS names

[Jeremy Allison]

On Thu, Feb 09, 2017 at 09:33:22AM +0100, Volker Lendecke wrote:
> On Wed, Feb 08, 2017 at 11:10:11AM -0800, Matthieu Patou wrote:
> > +static void resolve_one_name(void *private_data)
> > +{
> > +	struct dns_rr_srv *rr = private_data;
> > +	struct addrinfo *res = NULL;
> > +	struct addrinfo *p;
> > +	int num_ips = 0;
> > +	if (!interpret_string_addr_internal(&res, rr->hostname, 0)) {
> > +		return;
> > +	}
> > +	/* Add in every IP from the lookup. How many is that ? */
> > +	for (p = res; p; p = p->ai_next) {
> > +		if (num_ips != 0) {
> > +			if (NULL == (rr->ss_s = talloc_realloc(rr->ss_s, rr->ss_s,
> > +					struct sockaddr_storage, num_ips + 1)))
> > +			{
> > +				return;
> > +			}
> > +		}
> > +		memcpy(&rr->ss_s[num_ips], p->ai_addr, p->ai_addrlen);
> > +		if (is_zero_addr(&rr->ss_s[num_ips])) {
> > +			continue;
> > +		}
> > +		num_ips++;
> > +	}
> > +	rr->num_ips = num_ips;
> > +	if (res) {
> > +		freeaddrinfo(res);
> > +	}
> > +}
> 
> For my taste, this is much too complex for a thread job function. It
> calls minterpret_string_addr_internal, talloc_realloc and is_zero_addr
> where I'd have to think very long about whether they are safe to be
> called in a thread. With this change, interpret_string_addr_internal
> gets a completely fresh requirement that we would have to document
> very clearly: Thread safety. I would not hold my breath for us being
> good enough in our review process to always be alert that potentially
> thread-unsafe changes will be blocked for interpret_string_addr_internal.
> 
> That's what I meant with my comment to do only the absolute bare minimum
> in a threaded function to make sure it's thread-safe.
> 
> Can you restructure the code to only call getaddrinfo and nothing else
> in the threaded job.

+100 on what Volker said. Don't do *anything* more then the bare
minimum guarenteed thread-safe syscall/library call needed in the
thread function. Do the complex stuff in the callback instead.

You'll thank us later (or we'll thank ourselves later) when
the called functions change later to no longer be thread-safe,
and we get random crashes and failures :-).