Host/domain name, dns servers and default gateways information for Win/Linux #294

chikei · 2017-01-03T17:01:07Z

Hi,

here is interface for OS networking parameters and implementation for Linux/Win (sorry I don't have machine for other OS :().
Now it contains host name/dns domain name/dns servers/default gateway of IPv4/6.

Domain suffix search list is also considered but I think I'll wait JNA release VT_ARRAY support for this.

dbwiddis · 2017-01-03T18:32:05Z

Looks like you missed committing one of the changed files:

[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /home/travis/build/oshi/oshi/oshi-core/src/main/java/oshi/software/os/linux/LinuxOperatingSystem.java:[49,8] oshi.software.os.linux.LinuxOperatingSystem is not abstract and does not override abstract method getNetworkParams() in oshi.software.os.OperatingSystem

dbwiddis · 2017-01-03T20:53:47Z

oshi-core/src/main/java/oshi/software/os/linux/LinuxNetworkParams.java

+        int maxNameServer = 3;
+        List<String> servers = new ArrayList<>();
+        for(String line: resolv){
+            if(!line.startsWith(key)) continue;


Style: avoid the continue and break in the same loop. Just test for a true startsWith() here and put the rest of the method inside the if().

dbwiddis · 2017-01-03T20:58:22Z

oshi-core/src/main/java/oshi/software/os/linux/LinuxNetworkParams.java

+                value.charAt(0) == '#' || value.charAt(0) == ';') continue;
+            String val = value.split("[ \t#;]", 2)[0];
+            servers.add(val);
+            if(servers.size() >= maxNameServer) break;


Use brackets and separate lines. If you're using Eclipse IDE, use its code style for "Sun Java Conventions". Also change your "cleanup" style to remove unused imports that I see the codacy-bot has flagged.

dbwiddis · 2017-01-03T21:04:38Z

oshi-core/src/main/java/oshi/software/os/linux/LinuxNetworkParams.java

+            if(fields.length > 3 && fields[0].equals("::/0")) {
+                try{
+                    boolean isGateway = fields[2].indexOf('G') != -1;
+                    int metric = Integer.parseInt(fields[3]);


Use ParseUtil.parseIntOrDefault() which already handles the exception and will simplify your code.

dbwiddis · 2017-01-03T21:11:43Z

oshi-core/src/main/java/oshi/software/os/windows/WindowsNetworkParams.java

+     */
+    @Override
+    public String getIpv4DefaultGateway() {
+        String dest = "0.0.0.0/0";


Put this (and other similar throughout the PR) string as a constant at the top of the file.

dbwiddis · 2017-01-03T21:13:25Z

oshi-core/src/main/java/oshi/software/os/windows/WindowsNetworkParams.java

+        Map<String, List<Object>> vals = WmiUtil.selectObjectsFrom("ROOT\\StandardCimv2", "MSFT_NetRoute",
+            "NextHop,RouteMetric", "WHERE DestinationPrefix=\"" + dest + "\"", GATEWAY_TYPES);
+        List<Object> metrics = vals.get("RouteMetric");
+        if (vals.get("RouteMetric").size() == 0) {


Style/readability: use !isEmpty() and go straight to what's currently under else. Then just put the return ""; after the if.

dbwiddis

Overall looks good. You're missing one file needed for compiling (no doubt you made a local change and missed committing it).

See style comments by codacy-bot and this review; use a code formatting IDE if you can (if not I'll just do a pass after merging the PR).

dbwiddis · 2017-01-03T21:39:01Z

oshi-core/src/main/java/oshi/software/os/linux/LinuxNetworkParams.java

+     */
+    @Override
+    public String getHostName() {
+        String hn = FileUtil.getStringFromFile("/proc/sys/kernel/hostname");


Prefer native calls to reading files or parsing command lines. In this case, there is a gethostname() function in libc.

dbwiddis · 2017-01-03T21:41:14Z

oshi-core/src/main/java/oshi/software/os/linux/LinuxNetworkParams.java

+     */
+    @Override
+    public String getDomainName() {
+        return ExecutingCommand.getFirstAnswer("dnsdomainname");


Prefer native calls to reading files or parsing command lines. In this case, dnsdomainname is the result of calling getaddrinfo() on the result of gethostname().

dbwiddis

A few more comments. Note the code for getaddrinfo() and hostname() will be portable to Unix (Solaris and FreeBSD) as well.

dbwiddis · 2017-01-03T22:33:40Z

See comments on #269 for code which works on other OS's.

dbwiddis · 2017-01-03T23:23:31Z

Some more thoughts:

Why aren't we using built-in Java methods that already do all the cross-platform heavy lifting (probably using the same system calls I already suggested we use)? For example:

Host Name: just use InetAddress.getHostName()
Domain Name: just use InetAddress.getCanonicalHostName()

(As an aside, my initial objection to adding these to OSHI was because they were already available to users in core java. It makes sense to add them as part of an overall implementation including other things, though.)

For the gateway, netstat -nr is cross-platform (OSX, Linux, Unix, and even Windows, although the Windows output format is different) so you could implement that in a parent class, and override on Windows only with the WMI implementation, although the route command alternatives also work.

There is a non-API cross-platform Sun way of getting the nameservers:

        List<String> nameservers = (List<String>) sun.net.dns.ResolverConfiguration.open().nameservers();

But we shouldn't use non-API methods. Still, digging into the source code of their implementations may be instructive although the resolv.conf approach for *nix, WMI for WIndows, and scutil --dns for OSX are probably sufficient.

hazendaz · 2017-01-03T23:57:51Z

That sun package would not likely work with ibm jdk Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: dbwiddis <[email protected]> Sent: Tuesday, January 3, 2017 6:23:31 PM To: oshi/oshi Cc: Subscribed Subject: Re: [oshi/oshi] Host/domain name, dns servers and default gateways information for Win/Linux (#294) Some more thoughts: Why aren't we using built-in Java methods that already do all the cross-platform heavy lifting (probably using the same system calls I already suggested we use)? For example: * Host Name: just use InetAddress.getHostName() * Domain Name: just use InetAddress.getCanonicalHostName() (As an aside, my initial objection to adding these to OSHI was because they were already available to users in core java. It makes sense to add them as part of an overall implementation including other things, though.) For the gateway, netstat -nr is cross-platform (OSX, Linux, Unix, and even Windows, although the Windows output format is different) so you could implement that in a parent class, and override on Windows only with the WMI implementation, although the route command alternatives also work. There is a non-API cross-platform Sun way of getting the nameservers: List<String> nameservers = (List<String>) sun.net.dns.ResolverConfiguration.open().nameservers(); But we shouldn't use non-API methods. Still, digging into the source code of their implementations may be instructive although the resolv.conf approach for *nix, WMI for WIndows, and scutil --dns for OSX are probably sufficient. - You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#294 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA7ho0jxSD63dOrvcKuRTDcJI17buWvCks5rOthzgaJpZM4LZz93>.

dbwiddis · 2017-01-04T00:16:34Z

oshi-json/src/main/java/oshi/json/software/os/impl/NetworkPramsImpl.java

+import oshi.json.software.os.OSFileStore;
+import oshi.json.util.PropertiesUtil;
+
+public class NetworkPramsImpl extends AbstractOshiJsonObject implements NetworkParams{


Class name typo.

dbwiddis · 2017-01-04T00:19:37Z

oshi-json/src/main/java/oshi/json/software/os/impl/NetworkPramsImpl.java

+    @Override
+    public JsonObject toJSON(Properties properties) {
+        JsonObjectBuilder json = NullAwareJsonObjectBuilder.wrap(this.jsonFactory.createObjectBuilder());
+        if (PropertiesUtil.getBoolean(properties, "operatingSystem.networkParams.hostName")) {


Need to add all these property strings to the sample oshi.json.properties file in src/test/resources.

dbwiddis · 2017-01-04T00:32:56Z

Agreed we should not use the sun package. I was only suggesting it from a 'look at the source' standpoint. Which may be overkill. I did find a dnsjava project that does a lot of DNS-related stuff. But digging into the resolving code they don't have a significant improvement on command line parsing. They check /etc/resolv.conf, check the dns.server and dns.search system properties, or parse the output of ipconfig and platform-specific alternatives.

chikei · 2017-01-04T14:36:11Z

I was also hesitated on add host/dns domain name at beginning, it was then added because it's kind of weird having a network parameter but couldn't provide these two things. But like you said, maybe I should make getHostName as a InetAddress.getHostName wrapper. On the other hand, getCanonicalHostName resolves to IP address on my windows machine, so I'll try to using native lib (the nice getaddrinfo and WMI) .

As for the resolving code, actually it was following how the sun ResolverConfigurationImpl did. I just make it here because the sun one is non-API and it resolve to 5 name server instead of 3.

* using `InetAddress.getLocalHost().getHostName()` for getHostName * put resolv.conf parsing code to its getDnsServers

dbwiddis · 2017-01-04T18:47:59Z

Glad to hear you dug into the Sun class. As for the host/dns names, I'd default to theInetAddress wrappers. The Canonical name, especially, gives you a lot more "best effort" attempts than it'd be worth to write yourself.

resolv.conf should work on all *nix systems, including OSX. On OSX, while it is not used, it does look like the system keeps the file updated so it'd be available for information:

<danielwiddis> [01-03 17:06] ~/git/oshi $ cat /etc/resolv.conf
#
# Mac OS X Notice
#
# This file is not used by the host name and address resolution
# or the DNS query routing mechanisms used by most processes on
# this Mac OS X system.
#
# This file is automatically generated.
#
nameserver 8.8.8.8
nameserver 8.8.4.4

dbwiddis · 2017-01-04T19:04:26Z

My suggestion based on everything discussed so far:

Use what you have for Windows.
Make a common *nix parent class that
- Implements the InetAddress approach for host and dns name, or uses the libc system calls for getHostName() and getAddrInfo(); depending on how good the canonical name actually performs
- uses /etc/resolv.conf for nameservers
- parses the output of netstat -nr for default gateway.
Override the *nix parent in a few cases (e.g., when the dnshostname command is available, and the route command variants for gateway discussed in Feature request: Routing information & DNS settings #269)

dbwiddis · 2017-01-04T22:29:31Z

This StackOverflow post discusses why InetAddress.getCanonicalHostName sometimes returns numeric. Meanwhile, the source code for dnsdomainname shows how easy it is to get with system calls.

codecov-io · 2017-01-08T16:52:39Z

Current coverage is 84.62% (diff: 100%)

Merging #294 into master will not change coverage

@@             master       #294   diff @@
==========================================
  Files            23         23          
  Lines          1080       1080          
  Methods           0          0          
  Messages          0          0          
  Branches        195        195          
==========================================
  Hits            914        914          
  Misses           72         72          
  Partials         94         94

Powered by Codecov. Last update afe3587...4bf0d57

coveralls · 2017-01-08T16:52:42Z

Coverage remained the same at 92.222% when pulling 19e6546 on chikei:network-params into 4e654e6 on oshi:master.

chikei · 2017-01-08T17:08:12Z

@dbwiddis PTAL

Changes:

Using InetAddress.getHostName() for hostname
Parse /etc/resolv.conf for *nix
Using route for other *nix, (Linux stays the same because it outputs the same format as /proc/net/route)
Using getaddrinfo for *nix

dbwiddis · 2017-01-08T17:46:38Z

I'll take a look at this later this week. I see you've written a junit test. We'll also probably want one in SystemInfoTest as an example, but I'll probably work on that as I'm testing this so no need for you to do that.

Thanks for your work on this!

dbwiddis · 2017-01-09T00:40:56Z

A few quick comments... codacy found an unused LOG variable, and my IDE is flagging a few serializable classes that don't have the serial version ID.

dbwiddis

Looking good. Just a few more changes needed. See comments.

dbwiddis · 2017-01-14T20:20:56Z

oshi-core/src/main/java/oshi/software/os/windows/WindowsNetworkParams.java

+    }
+
+    private String getNextHop(String dest) {
+        Map<String, List<Object>> vals = WmiUtil.selectObjectsFrom("ROOT\\StandardCimv2", "MSFT_NetRoute",


The ROOT\\StandardCimv2 requires Windows 8+. This call produces errors on Windows 7.

We need to provide a fallback method for finding the default gateway on Win7. My suggestion would be to parse the output of netstat -nr and return the gateway for 0.0.0.0 (IPv4) and ::/0 (IPv6).

dbwiddis · 2017-01-14T20:24:17Z

oshi-core/src/main/java/oshi/software/os/windows/WindowsNetworkParams.java

+            LOG.error("Failed to get dns domain name. Error code: {}", Kernel32.INSTANCE.GetLastError());
+            return "";
+        }
+        return new String(buffer);


On my test VM, buffer is filled with null characters, so the String results in "\u0000\u0000\u0000\ ... 256 times ... \u0000". You should return new String(buffer).trim();.

dbwiddis · 2017-01-14T20:40:01Z

oshi-core/src/main/java/oshi/software/common/AbstractNetworkParams.java

+     */
+    @Override
+    public String getDomainName() {
+        LibC.Addrinfo hint = new LibC.Addrinfo();


These LibC calls here are specific to Unix, and this code will only work on FreeBSD and Solaris. Either this Unix-specific should be implemented in a Unix class that accept the unix lass you imported, or you should do an OS check and import the appropriate LibC to use (the Linux LibC is a different class, and SystemB is the equivalent for the OS X case).

dbwiddis · 2017-01-14T20:41:17Z

oshi-core/src/main/java/oshi/software/common/AbstractNetworkParams.java

+        LibC.Addrinfo hint = new LibC.Addrinfo();
+        hint.ai_flags = LibC.AI_CANONNAME;
+        String hostname = null;
+        try {


Why not just call this class's own getHostName() here, where you've implemented this code?

dbwiddis · 2017-01-14T20:43:16Z

oshi-core/src/main/java/oshi/software/os/mac/MacNetworkParams.java

+     */
+    @Override
+    public String getDomainName() {
+        return "";


This overrides the default methods we put in the abstract class. If you get the correct LibC/SystemB class to create an instance of in that class, you can just leave this blank and OS X will use the C functions.

dbwiddis · 2017-01-14T20:44:16Z

oshi-core/src/main/java/oshi/software/os/mac/MacNetworkParams.java

+     * {@inheritDoc}
+     */
+    @Override
+    public String[] getDnsServers() {


You're overriding the default method of reading /etc/resolv.conf. Remove this override.

dbwiddis · 2017-01-14T20:53:24Z

oshi-core/src/main/java/oshi/software/os/mac/MacNetworkParams.java

+        boolean v6Table = false;
+        for(String line: lines){
+            if(v6Table && line.startsWith(DEFAULT_GATEWAY)){
+                String[] fields = line.split("[ \t#;]");


You need a + after the regex here to split on the multiple whitespace. You could also just use "\\s+".

Also if you do decide to use the \t you would need a second backslash to escape the escape.

coveralls · 2017-01-15T01:03:59Z

Coverage remained the same at 92.222% when pulling 4bf0d57 on chikei:network-params into afe3587 on oshi:master.

chikei added 4 commits January 3, 2017 01:09

Add NetworkParams interface and implements linux/windows

72fe23f

Fix fields splitting regex

7fb3fbe

Add dummy implements and integrate into OperatingSystem interface

110764a

Add corresponding json part

4e420f6

dbwiddis reviewed Jan 3, 2017

View reviewed changes

dbwiddis requested changes Jan 3, 2017

View reviewed changes

dbwiddis mentioned this pull request Jan 3, 2017

Feature request: Routing information & DNS settings #269

Closed

dbwiddis reviewed Jan 4, 2017

View reviewed changes

chikei added 5 commits January 4, 2017 22:38

commit missing linux part

56d5f30

style fixing

092e65b

fix class name typo

af3fc3b

add json properties to sample file

0eea0d5

Using AbstractNetworkParams

a1c5bb2

* using `InetAddress.getLocalHost().getHostName()` for getHostName * put resolv.conf parsing code to its getDnsServers

chikei added 2 commits January 7, 2017 00:29

Implements gateway for Mac/Solaris/FreeBsd

f6ecd61

Use native funtions for dns domain name

19e6546

dbwiddis approved these changes Jan 9, 2017

View reviewed changes

dbwiddis requested changes Jan 14, 2017

View reviewed changes

dbwiddis added 4 commits January 14, 2017 16:46

Update SystemInfoTest.java

366fc2d

Update oshi-json SystemInfoTest.java

cf19954

Update CHANGELOG.md

9b8058d

Merge branch 'master' into network-params

4bf0d57

dbwiddis merged commit e422623 into oshi:master Jan 26, 2017

Host/domain name, dns servers and default gateways information for Win/Linux #294

Host/domain name, dns servers and default gateways information for Win/Linux #294

Conversation

chikei commented Jan 3, 2017

dbwiddis commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis commented Jan 3, 2017

dbwiddis commented Jan 3, 2017

hazendaz commented Jan 3, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis commented Jan 4, 2017

chikei commented Jan 4, 2017 • edited Loading

dbwiddis commented Jan 4, 2017

dbwiddis commented Jan 4, 2017

dbwiddis commented Jan 4, 2017

codecov-io commented Jan 8, 2017 • edited Loading

Current coverage is 84.62% (diff: 100%)

coveralls commented Jan 8, 2017

chikei commented Jan 8, 2017

dbwiddis commented Jan 8, 2017 • edited Loading

dbwiddis commented Jan 9, 2017

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2017

chikei commented Jan 4, 2017 •

edited

Loading

codecov-io commented Jan 8, 2017 •

edited

Loading

dbwiddis commented Jan 8, 2017 •

edited

Loading