Feature/get local interfaces #1350

zalintyre · 2020-10-04T21:39:28Z

Hello,

here is my pull request for #1345. Feel free to leave feedback :)

Franz

(According to checkstyle configuration)

…es, too To keep the API stable, getNetworkIFs behavior is unchanged.

coveralls · 2020-10-04T21:48:03Z

Coverage remained the same at 88.098% when pulling c05124b on zalintyre:feature/get-local-interfaces into 50ff1ea on oshi:master.

zalintyre · 2020-10-04T21:49:34Z

It seems like the tests are quite system-dependent. Here are the current failures from AppVeyor:

[ERROR] oshi.hardware.NetworksTest.testAllNetworkInterfaces  Time elapsed: 0.572 s  <<< FAILURE!
java.lang.AssertionError: NetworkIF MTU should not be negative
	at oshi.hardware.NetworksTest.testAllNetworkInterfaces(NetworksTest.java:123)
[ERROR] oshi.hardware.NetworksTest.testInetNetworkInterfaces  Time elapsed: 10.74 s  <<< FAILURE!
java.lang.AssertionError: NetworkIF MTU should not be negative
	at oshi.hardware.NetworksTest.testInetNetworkInterfaces(NetworksTest.java:71)

On my Linux development machine, I get a test failure because OperatingSystemTest.testGetServices doesn't report stopped services.

dbwiddis

Thanks for the PR! It looks great. A few changes I'd like:

I'm not really happy with the term "inet" used in the javadocs. I'd prefer a more verbose term to represent the "not loopback or virtual" nature.
Rather than a getX() and getAllX() pair, let's overload the methods. The new method will add a boolean argument, the old method will keep the no-arg version. In the Abstract object you can implement the no-arg delegating to the arg version with false. Then in the platform-specific code you only need to implement one method, with a boolean argument you can switch on.

.editorconfig

dbwiddis · 2020-10-05T00:35:14Z

oshi-core/src/main/java/oshi/hardware/HardwareAbstractionLayer.java

@@ -77,13 +77,22 @@
    List<HWDiskStore> getDiskStores();

    /**
-     * Gets a list of {@link NetworkIF} objects, representing a network interface
+     * Gets a list of inet {@link NetworkIF} objects, representing a network interface


I'd prefer not to add inet here as it's not clearly defined, and the object returned is part of the OSHI API.

I see throughout the changes you've used this term, though, so perhaps you need to just clearly define what "inet" means. Or use a more verbose term that's more clear.

dbwiddis · 2020-10-05T00:38:07Z

oshi-core/src/main/java/oshi/hardware/HardwareAbstractionLayer.java

+     * @return An {@code UnmodifiableList} of {@link NetworkIF} objects representing
+     *         the interfaces
+     */
+    List<NetworkIF> getAllNetworkIFs();


Instead of a new name, let's just overload the original one. Make this second method getNetworkIFs(boolean includeLoopbackAndVirtual); (or another shorter name that means the same). Calling the first method will simply redirect to getNetworkIFs(false) and then you only have to really write the implementations for one method.

I did not overload the method on purpose, following "Clean Code"'s statement about boolean parameters: https://medium.com/@amlcurran/clean-code-the-curse-of-a-boolean-parameter-c237a830b7a3

If you want, I can change it, though.

Thanks for that article link. I did spend some time reading it and considering this issue, and present the following counter for discussion.

While the article makes great points, it's dealing primarily with mutually exclusive control flows. That, however, is not the case here. We are using overlapping/nearly-identical flows (iterating interfaces), with the only difference being a filtering parameter. We are not making the code any more complex than it already is; the existing code includes this conditional:

if (!netint.isLoopback() && netint.getHardwareAddress() != null) { .... }

All you'd do is add the boolean parameter (or its opposite) to that conditional.

Another consideration is API consistency. We use exactly the pattern I'm proposing in iterating Filesystems (see FileSystem.getFileStores(boolean localOnly)). Of course, that's now complicated by the fact that there's a request to not filter the overlay filesystem types ( #1339 ), so perhaps a boolean is a poor choice there, or we may need a second parameter (probably a collection of strings). Also, the OperatingSystem class has a default getProcesses() which gets all processes, delegating to getProcesses(int limit, ProcessSort sort) for the filtering/sorting/size limiting output. I see this as a very similar type of application.

More importantly, the API is designed to transparently work with the Jackson project's ObjectMapper (see the Json class in the oshi-demo project). By default, this class automatically, via reflection, assumes a get*() method maps directly to a field. Perhaps using get*() is a bad choice to begin with, but at this point we're stuck with the API. Having two no-arg get*() methods will duplicate the JSON output. The mapper does ignore the "getters" with arguments, and would ignore a method name that doesn't look like a getter (which is the reason queryNetworkInterfaces() is not getNetworkInterfaces().)

So I'm not overly opposed to you using two separate methods if you make the "full method" a query*() one, and then filter from those results in the get*() method. That'd be an implementation detail. However, given the similarity to the existing API implementations, I'd prefer to just have a boolean filter parameter on whether to include the loopback interfaces.

Also, complicating things, in the existing method there are two filters, the loopback and the virtual (whether a hardware address exists). So perhaps boolean isn't the best filtering, but open to ideas on how to make this both flexible and not overly complex (I don't want two booleans). I do think an "unfiltered" output vs. a "most common filters" with a boolean switch is a good compromise.

oshi-core/src/main/java/oshi/hardware/common/AbstractNetworkIF.java

dbwiddis · 2020-10-05T00:45:01Z

oshi-core/src/main/java/oshi/hardware/common/AbstractNetworkIF.java

+            LOG.error("Socket exception when retrieving interfaces: {}", ex.getMessage());
+        }
+
+        return new ArrayList<>();


Use Collections.emptyList() here.

dbwiddis · 2020-10-05T00:51:36Z

oshi-core/src/test/java/oshi/hardware/NetworksTest.java

        SystemInfo si = new SystemInfo();

-        for (NetworkIF net : si.getHardware().getNetworkIFs()) {
+        for (NetworkIF net : si.getHardware().getAllNetworkIFs()) {


Given one list is the superset of the other, it seems like we don't need to repeat all the tests, just do the "all" version for the detail and do at least one test for the "non-local/virtual" version to show the result isn't loopback, etc.

dbwiddis · 2020-10-05T01:05:41Z

Interesting that appveyor fails now but didn't before. (It just passed tests on the master branch.)

Even more interesting is that the failure, getMTU(), is in the Java class. I wonder if it's negative or just a very large (unsigned) value being interpreted as negative. Is this only associated with the loopback and virtual interfaces? Perhaps it should not be tested if it's not relevant.

The linux failure seems like it should be excluded in that condition, can you submit a separate issue or PR for that?

dbwiddis · 2020-10-05T22:29:43Z

FYI, the macos failures may be a separate bug that I'm hunting down in the tcpstats structure. You can ignore them.

zalintyre · 2020-10-06T18:31:59Z

I refactored the getNetworks() method. I tried to keep the API stable, though.

dbwiddis

Looks good and works. I have two more requests and this will be ready to merge!

DRY: just implement the no-arg to "false" delegation once in the AbstractHAL, and save yourself 10 other method calls.
Move the boolean test outside of the loop (only test once) and move the exception handling to where it's thrown. This is logically the same as what you had, but lets us simplify the method.

oshi-core/src/main/java/oshi/hardware/common/AbstractNetworkIF.java

oshi-core/src/main/java/oshi/hardware/platform/linux/LinuxHardwareAbstractionLayer.java

dbwiddis · 2020-10-07T19:59:33Z

Looks good with the latest commit! I'll do some final cleanup and merge!

zalintyre added 2 commits October 4, 2020 22:26

Add .editorconfig to disallow grouped imports

3a876e2

(According to checkstyle configuration)

[#1345] Add getAllNetworkIFs method to include local network interfac…

72209b4

…es, too To keep the API stable, getNetworkIFs behavior is unchanged.

dbwiddis requested changes Oct 5, 2020

View reviewed changes

dbwiddis added the hacktoberfest-accepted Accepted during Hacktoberfest label Oct 5, 2020

dbwiddis linked an issue Oct 5, 2020 that may be closed by this pull request

Resulting List<NetworkIF> does not include loopback and virtual interfaces #1345

Closed

zalintyre added 5 commits October 5, 2020 19:55

Return Collections.emptyList instead of ArrayList

6616b2c

Rename "inet" to "non-local"

68c5eb0

Simplify network interfaces tests

93befc9

Test for non-negative MTU only on non-local network interfaces

b03278e

Add comments and useful default settings to editorconfig

660f479

Provide overloaded version of getNetworks()

5ef0a10

dbwiddis reviewed Oct 6, 2020

View reviewed changes

oshi-core/src/main/java/oshi/hardware/common/AbstractNetworkIF.java Outdated Show resolved Hide resolved

oshi-core/src/main/java/oshi/hardware/platform/linux/LinuxHardwareAbstractionLayer.java Outdated Show resolved Hide resolved

zalintyre added 2 commits October 7, 2020 21:12

Simplify network interface filter logic

8343e30

Remove getNetworkIFs() by implementation in abstract base class

7c47e6b

dbwiddis approved these changes Oct 7, 2020

View reviewed changes

Remove unused methods, formatting, chaneglog

c05124b

dbwiddis merged commit 493d109 into oshi:master Oct 7, 2020

zalintyre deleted the feature/get-local-interfaces branch October 7, 2020 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/get local interfaces #1350

Feature/get local interfaces #1350

zalintyre commented Oct 4, 2020

coveralls commented Oct 4, 2020 •

edited

Loading

zalintyre commented Oct 4, 2020

dbwiddis left a comment

dbwiddis Oct 5, 2020

dbwiddis Oct 5, 2020 •

edited

Loading

zalintyre Oct 5, 2020

dbwiddis Oct 5, 2020

dbwiddis Oct 5, 2020

dbwiddis Oct 5, 2020

dbwiddis Oct 5, 2020

dbwiddis commented Oct 5, 2020 •

edited

Loading

dbwiddis commented Oct 5, 2020

zalintyre commented Oct 6, 2020

dbwiddis left a comment

dbwiddis commented Oct 7, 2020

Feature/get local interfaces #1350

Feature/get local interfaces #1350

Conversation

zalintyre commented Oct 4, 2020

coveralls commented Oct 4, 2020 • edited Loading

zalintyre commented Oct 4, 2020

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis Oct 5, 2020

Choose a reason for hiding this comment

dbwiddis Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

zalintyre Oct 5, 2020

Choose a reason for hiding this comment

dbwiddis Oct 5, 2020

Choose a reason for hiding this comment

dbwiddis Oct 5, 2020

Choose a reason for hiding this comment

dbwiddis Oct 5, 2020

Choose a reason for hiding this comment

dbwiddis Oct 5, 2020

Choose a reason for hiding this comment

dbwiddis commented Oct 5, 2020 • edited Loading

dbwiddis commented Oct 5, 2020

zalintyre commented Oct 6, 2020

dbwiddis left a comment

Choose a reason for hiding this comment

dbwiddis commented Oct 7, 2020

coveralls commented Oct 4, 2020 •

edited

Loading

dbwiddis Oct 5, 2020 •

edited

Loading

dbwiddis commented Oct 5, 2020 •

edited

Loading