I just noticed that I get gai_strerror_ptr dynamically, but then called
gai_strerror directly and incorrectly.
Here's a more robust way to get the error string:
const char *error_string =
(gai_strerror_ptr == NULL) ? NULL : (*gai_strerror_ptr)(gai_error);
if (error_string == NULL)
error_string = "unknown error";
Webrev regenerated.
Martin
On Wed, Sep 1, 2010 at 10:51, Martin Buchholz <marti...@google.com> wrote:
>
>
> On Wed, Sep 1, 2010 at 06:27, Michael McMahon <
> michael.x.mcma...@oracle.com> wrote:
>
>> Martin Buchholz wrote:
>>
>>> Hi net maintainers,
>>>
>>> I'd like you to do a code review.
>>>
>>> I got frustrated with incomplete information from exceptions thrown from
>>> java.net <http://java.net>, so I hacked up some fixes for them. This is
>>> the result.
>>>
>>>
>>> generify; remove compiler warnings, typos, casts; return status
>>> information via gai_strerror when getaddrinfo fails
>>>
>>> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/netErrors/<http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/netErrors/><
>>> http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk7/netErrors/>
>>>
>>> But... I don't know what I'm doing here, and the underlying bug I was
>>> tracking down is fixed, so it's hard for me to test my code changes. It
>>> would be good if a java.net <http://java.net> maintainer would add a
>>> test case, or at least verify my exception improvements with an ad hoc test.
>>>
>>> Thanks,
>>>
>>> Martin
>>>
>> Hi Martin,
>>
>> Looks like a useful set of changes. I can confirm the exception text is
>> improved on Solaris and
>> Linux. I'm not sure there is much value in adding a regression test case
>> that checks for specific
>> exception text string though ...
>>
>>
> I try to do something like that in ProcessBuilder tests, but it can't be
> done very portably.
>
> } catch (IOException e) {
> String m = e.getMessage();
> if (EnglishUnix.is() &&
> ! matches(m, "Permission denied"))
>
> but I'm not going to do that here.
>
>
>
>> On the source change itself:
>>
>> InetAddress.java: I'd prefer to see the type parameters used in the places
>> where you
>> rely on jdk7 type inference. I don't think clarity is enhanced in these
>> cases, since the parameters
>> are quite straight forward.
>>
>
> As you wish.
>
>
>>
>> Variable "address" could be renamed "addresses" since we know now it
>> refers to an array.
>>
>>
> I wasn't going to do that, because that would trigger many other address =>
> addresses changes. But since you asked, I went ahead and did that, and
> changed the names of some of the private methods.
>
> Webrev regenerated.
>
> Martin
>
>
>> Other than that, looks fine.
>>
>> - Michael
>>
>
>
