Request for comment for async resolution of DNS names
Matthieu Patou
mat at samba.org
Thu Feb 9 19:59:15 UTC 2017
On 02/09/2017 11:30 AM, Jeremy Allison wrote:
> On Thu, Feb 09, 2017 at 11:18:37AM -0800, Matthieu Patou wrote:
>> Volker,
>> On 02/09/2017 12:33 AM, 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.
>> Thanks for the feedback, I wasn't sure that if I called getaddrinfo
>> directly someone wouldn't argue that interpret_string_addr_internal was
>> meant for that and that I should use it.
> interpret_string_addr_internal() calls strchr_m() which calls
> get_iconv_handle(). I am 100% sure get_iconv_handle() is not
> thread-safe.
I'm fine, I guess we also don't want to have any DEBUG statement in the
thread ?
--
Matthieu Patou
Samba Team
http://samba.org
More information about the samba-technical
mailing list