java.net.NetworkInterface.getNetworkInterfaces does not work properly on AIX with IPv6

[Jonathan Lu]

On 10/20/2011 09:16 PM, Neil Richards wrote:
> On Thu, 2011-10-20 at 17:12 +0800, Jonathan Lu wrote:
>> On 10/20/2011 02:35 PM, Steve Poole wrote:
>> Thanks Steve, I've updated the test case and patch, see the attachments.
>> I've added IBM portions copyright comment to both headers.
>>
>> - Jonathan
> For ease of review, I've uploaded the suggested fix as a webrev [1].
>
> Whilst creating this, I stripped the (large amount of) extraneous
> trailing whitespace characters from the lines added by the patch.
> (It would be helpful to check for this when posting patches).
>
Thanks Neil for creating that webrev.
I'm sorry for the whitespace issue, I did not 'set list' in my VIM 
editor before, will turn on that option from now on. Attachment 
patch.diff is an updated version.
> Looking at the change, I have some concerns.
>
> Firstly, the added AIX-specific code (between lines 1102-1187) has been
> placed in a block of code that looks to be Linux-specific (lines
> 1013-1365, protected by '#ifdef __linux__').
>
> So it's unclear why any of this code will be used by AIX.
>
To be clear, I've moved the AIX code of enumIPv6Interfaces to a stand 
alone '#ifdef AIX' block.
> Secondly, if an exception occurs whilst the list of interfaces is being
> compiled, both Linux and Solaris look to return the list compiled up to
> the point of the exception (blocks starting at lines 1222&  1505).
>
> In the suggested AIX code, however, the compiled interface list is freed
> up in this situation, and NULL returned (block starting at line 1174).
>
> It seems to me that these routines should present similar semantics
> (across the 3 platforms).
> As the interface list returned is an augmentation of an interface list
> that is originally given to the function (the original value of 'ifs'),
> the suggested AIX code may free off entries in the list that it was not
> responsible for allocating, which runs the risk of resulting in
> duplicate calls to free().
>
> Could you please investigate where the responsibilities properly lie for
> allocation and deallocation of the entries in the interface list given
> to the enumIPv6Interfaces() function, then update the proposal /
> comments appropriately?
For the exception handling part, I think AIX implementation works in the 
same way as enumIPv4Interfaces on Linux platform.

Although the interface list returned is an argument given to this 
function (known as 'ifs'), this pointer is modified by addif() function 
which may add a new node to the head of the list, so the value of this 
pointer changes in this function.
And if NULL is return by AIX version enumIPv6Interfaces(), then the 
interface list pointer 'ifs' from caller function (here is 
'enumInterfaces') will be set to NULL which will just skip the next 
freeif() operation. So from my point of view, I do not see any potential 
duplicate calls to free().

> Also, I'm concerned that the name of the testcase provided is overly
> vague (what _specifically_ about NetworkInterface does it test?), and
> that the @summary description does not describe what the intention of
> the test is.
> In particular, there is nothing platform-specific about the testcase, so
> I would not expect its description to be in terms of something
> AIX-specific.
>
Yes, the testcase is nothing platform-specific, I've updated the name 
and summary.
How about changing the name to NetworkInterfaceUniquenessTest?
> Hope this helps,
> Regards, Neil
>
> [1]http://cr.openjdk.java.net/~ngmr/ojdk-172/webrev.00/
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: NetworkInterfaceUniquenessTest.java
Type: text/x-java
Size: 2549 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/net-dev/attachments/20111021/db603ed0/NetworkInterfaceUniquenessTest.java 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 2942 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/net-dev/attachments/20111021/db603ed0/patch.diff