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:
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.