[#958652] Set EAI_SYSTEM only when h_errno is NETDB_INTERNAL

[Carlos O'Donell]

On 05/21/2013 01:24 AM, Siddhesh Poyarekar wrote:
> On Thu, May 16, 2013 at 07:20:22PM +0530, Siddhesh Poyarekar wrote:
>>> What would help in the review of this, and the upstream review, is
>>> to have a detailed breakdown of the analysis you did while fixing
>>> this.
>>>
>>> Do you have such an analysis?
>>
>> I don't have a written detailed breakdown right now, but I don't mind
>> doing it.  I'll post an analysis of this soon.  Meanwhile, here's the
>> original patch which caused this breakage, in case that helps build
>> some of the context:
>>
>> http://sourceware.org/ml/libc-alpha/2012-10/msg00611.html
>>
> 
> OK, so here's the background of all these changes:
> 
> The original request was to have getaddrinfo return EAI_SYSTEM if the
> errno from the resulting request was EMFILE or ENFILE during the DNS
> host resolution, to differentiate those conditions from the usual
> EAI_NONAME.  The reason behind this was that they could be treated
> differently from the default error conditions.  To satisfy this, I
> looked at the two places that these were implemented,
> i.e. get_XX_by_YY.c (for AF_INET) and dns-host.c (for AF_UNSPEC and
> AF_INET6).  The places that set this errno were during module load
> and/or during socket creation in both those places.  I set nss_status
> to NSS_STATUS_UNAVAILABLE in those cases to signify that the error was
> something other than a standard failure and used it in gaih_inet() to
> set the return code as EAI_SYSTEM.
> 
> There were two problems with that patch.  The last part (of setting
> NSS_STATUS_UNAVAILABLE and checking on it) was wrong since the default
> failure was NSS_STATUS_UNAVAILABLE.  As a result, all failures were
> returning EAI_SYSTEM instead of EAI_NONAME.  The correct check here
> would have been to look for h_errno to be NETDB_INTERNAL since I set
> that explicitly.  Secondly, I had not checked for module load errors
> due to AF_UNSPEC or AF_INET6, which would occur in gaih_inet() itself.
> This needed the second patch (i.e. the one I'm requesting review for)
> which checks h_errno to check failure and ensures that ENOENT does not
> qualify for an EAI_SYSTEM, since the latter case is documented to be a
> valid reason for EAI_NONAME.
> 
> Finally, the tweak for #958652.  This was a minor oversight TBH - I
> was testing for errno != ENOENT, but the module open code resets errno
> to 0 if there is a failure in dlopen'ing the module.  As a result this
> check also needs a check to ensure that errno is actually set, to
> signal a failure.
> 
> I hope this is sufficient.  Please let me know if you need any further
> clarification.

That's perfect and exactly what I needed to look over and review the
code changes.

Cheers,
Carlos.