Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added IPAddress#toInetAddress() convenience method #437

Merged
merged 2 commits into from
Oct 22, 2015

Conversation

ronaldchl
Copy link
Contributor

@bsn-abat
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins.bigswitch.com//job/loxigen_pull_req/192/
Test PASSed.

throw new IllegalArgumentException(
"Error getting InetAddress for the IPAddress " + this, e);
}
if (inetAddress == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InetAddress.getByAddress never returns null, remove this check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't look at their implementation. Makes sense. Thanks!

@ronaldchl
Copy link
Contributor Author

Fixed in 3c99cdb. Thanks @andi-bigswitch

@bsn-abat
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins.bigswitch.com//job/loxigen_pull_req/193/
Test PASSed.

IPAddress<?> ip1 = IPAddress.of("2001:db8:abcd::1:2:3:4");
InetAddress ia0 = ip0.toInetAddress();
InetAddress ia1 = ip1.toInetAddress();
assertEquals(ia0, InetAddress.getByName("201.202.3.4"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that might be good to double check is that we don't trigger any network interaction here (i.e., this unit test doesn't hang forever if you take the computer off the Internet).

I am always a little bit scared of those InetAddress classes talking to the net by themselves...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as you don't pass a hostname you should be good WRT DNS requests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the source code.

// if host is an IP address, we won't do further lookup
if (Character.digit(host.charAt(0), 16) != -1 || (host.charAt(0) == ':')) {

}

@andi-bigswitch
Copy link
Contributor

Otherwise LGTM - feel free to self-*ck

@ronaldchl
Copy link
Contributor Author

SELF-ACK

@bsn-abat
Copy link

ABAT: START_MERGE: Log file at http://vss1/abat/2015.10.22.1938-m.loxigen.master/abat.log

bsn-abat pushed a commit to floodlight/loxigen-artifacts that referenced this pull request Oct 22, 2015
Loxigen Head commit floodlight/loxigen@8b8ac26
commit 8b8ac26edf425e61e34ec4a2e05f6505034569e2
Merge: 4d76743 3c99cdb
Author: abat <abat@bigswitch.com>
Date:   Thu Oct 22 12:38:19 2015 -0700

    Merge into master from pull request #437:
    Added IPAddress#toInetAddress() convenience method (floodlight/loxigen#437)

commit 3c99cdb2767001373decc7a3277a676d26aed74e
Author: Ronald Li <ronald.li@bigswitch.com>
Date:   Thu Oct 22 11:07:27 2015 -0700

    Implement IPAddress#toInetAddress() directly in subclasses as suggested
    by @andi-bigswitch

commit 79e06021411f96bdee9862342aab4b3985228036
Author: Ronald Li <ronald.li@bigswitch.com>
Date:   Wed Oct 21 16:29:04 2015 -0700

    Added IPAddress#toInetAddress() convenience method
@bsn-abat
Copy link

ABAT: ACCEPT: Successfully merged

In case you want to see the build log, check out :
Log file at http://vss1/abat/2015.10.22.1938-m.loxigen.master/abat.log
/cc

@bsn-abat bsn-abat merged commit 3c99cdb into floodlight:master Oct 22, 2015
bsn-abat pushed a commit that referenced this pull request Oct 22, 2015
Added IPAddress#toInetAddress() convenience method (#437)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants