Request for comment for async resolution of DNS names
Volker Lendecke
vl at samba.org
Thu Feb 9 08:33:22 UTC 2017
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.
Thanks,
Volker
More information about the samba-technical
mailing list