Initial reference implementation

[cilki]

Far from 100% finished, but here's what I have so far. The most important files to look at are:

• reference/definitions.json
• reference/src/main/java/oshi/api/OSHI.java
• reference/src/main/java/oshi/driver/SystemDriverLinux.java
• reference/src/main/java/oshi/driver/hardware/disk/DiskDriverLinux.java
• reference/gen/main/java/oshi/api/hardware/disk/DiskContainer.java

I implemented rudimentary fetching logic for Nic, Disk, and Firmware on Linux only (this stuff is just for demonstration and should be rewritten later).

Note about the definitions file format:

I decided to use JSON rather than YAML for the definitions file for several reasons. The content also evolved from our initial specification. For example, I moved permissions information out of the definitions file since that's more of an implementation detail. I'll update the specification in the README eventually.

Package Survey

gen/main/java (generated API)

Container

• Object that stores last fetched values and can update its values.
• Fields are public, but hidden behind an interface to prevent the user from accessing
• Can exist in two states: ATTACHED (is holding onto a driver instance and therefore can update attributes) and DETACHED (doesn't have a driver instance). Containers can become detached if they are serialized then deserialized.
• Easy to convert to JSON with Jackson (we can easily add the annotations if necessary).

src/main/java (non-generated API, drivers, and examples)

Query Method

• A method responsible for fetching one or more attributes of a particular component instance.
• Currently always void, but could be made to return a status indicator.

Driver

• An object consisting of query methods
• Corresponds to exactly one component
• Corresponds to exactly one platform

Extension Driver

• A driver that may require external software to work
• Corresponds to exactly one extension

Big Picture

The main idea behind the design is to make the query methods first-class citizens (since that's OSHI's business logic afterall). Query methods are organized into driver classes for each platform (or extension). For example, the DiskDriverLinux contains query methods for the basic attributes of a disk. An extension like DiskDriverLinuxSMART relies on the presence of the smartmontools package and provides additional attributes. Attributes can be queried with whatever granularity we choose because each query method can be responsible for more than one attribute.

When building a driver to attach to a container at runtime, the available query methods are organized into a stack. When the container needs to fetch an attribute, the driver invokes each query method in the stack until one succeeds. This is implemented with the MethodHandle API, so reflection overhead is minimal.

Although driver classes can contain anything (including closable resources), they must have at least one query method. Additionally:

• Every query method is annotated with an automatically generated @Query annotation that defines one or more attributes that the method is responsible for fetching.
• Every query method is private void and can throw any exception. To update attributes, the method directly changes fields in the container.
• Query methods that require elevated permissions are annotated with @RequiresRoot and are only called if the permission requirements are satisfied.
• Query methods can optionally define fallback methods with @Fallback.

Remaining Issues

There are at least a few unsolved issues remaining:

• Closable driver resources (only necessary for PDH counters) are not handled. Java 9's Cleaner API should be able to release resources without having to force users to use try-with-resources for every component.
• Compositions of components should work in theory, but are not yet implemented.
• Extension attributes exist in the same container as normal attributes. This isn't ideal, but also not necessarily a problem either. Since Java doesn't have multiple inheritance, separating the extension attributes into separate containers (similar to platforms) would be impossible. My suggestion is to leave it and generate comments letting the user know that additional software is required to query those attributes.
• I'm unsure of how OshiResult fits into the design. Currently a null value for an attribute in the container represents "not present". If a query method fails along with all of its fallbacks, I'm not sure what to do.

Anyway, I'm happy to make any changes, big or small 👍. I'm also writing a paper with a more detailed explanation of everything that will be done by the middle of the month.

[dbwiddis]

Made a few comments in the code, mostly revolving around what to do when we can't do stuff, which is a big problem that I'm not sure you've completely thought through end-to-end handling. There are a few use cases that need specific (and possibly different) approaches:

• Some value simply isn't supported/available on that platform (e.g., Windows doesn't have inodes.)
• Some value simply isn't supported/available on that operating system flavor/version (e.g., 32/64 bit differences, Linux distros, etc.)
• Some value is supported but produces different results based on your operating system flavor/version or permissions or other installed/running software, etc.
• Some value is supported but occasionally fails to return a value due to comms/driver hiccups
• Some value has a completely different meaning on some hardware (see most recent raspberry pi updates) or software (the concept of a "service" on windows vs. other OS's).
• Some value should be treated as other than its return type (e.g., Strings representing dates, Strings of hex characters representing bytes, or Java long representing C unsigned long values, etc.

Why gen/main/java vs. src/gen/java or src/main/gen? Is this a standard location for code-generating? Do various IDEs know about this and offer easy configuration for it?

Status indicator on query method -- I like the idea of returning an int (error codes, anyone?) or an enum of the various types of OSHI hiccups that might happen (timed out, missed sensor reading, process terminated, etc.)

@RequiresRoot needs a bit more fine grain. Consider getting the ProcessorID value on Linux. You either need root or the cpuid package installed.

I'm not sure what you mean by closable resources, or why PDH Counters are an exception or are different than other resources (WMI, SMC, Core Foundation dictionaries, file reading, etc.)

OshiResult -- perhaps the return value mentioned above can handle this nicely. Return 0 if it's a normal result, 1 if it's a failure (probably null), 2 if it requires casting/parsing for further interpretation, etc. Or use an enum.

[dbwiddis]

We need a finer grain measure of compatibility, even within operating systems. For example, the "Up Time" counter on early versions of Windows is 32-bit and rolls over at ~49 days. Other attributes are only possible with add-on extensions or elevated permissions, etc.

[cilki]

I have an extension field that encodes an attribute's requirement of external add-ons, but I don't do anything with it in the drivers yet.

I removed permissions requirements from the spec because it's an implementation detail and makes more sense to declare it as an annotation on the method that requires permissions.

The problem with the incompatible field that we came up with in the initial YAML spec is that it pushes the attribute down into platform-specific territory. I'll add a follow up comment with a possible solution for this that I came up with.

As for the uptime example, you're saying that maybe the attribute should be compatible only on later Windows? I'm not sure how this could be done without arbitrating WindowsXP classes for example (which would be a nightmare).

[dbwiddis]

It's less a compatibility than a "document the results" thing. I'm just recalling there are many places in the Javadocs where I'm commenting about what you can and can't get on various OS's.

[dbwiddis]

If we're committing generated code we need to synchronize this header with the standard license header add-on.

[cilki]

What do you mean by synchronize? This comment should appear right below the license header and before the package declaration in all generated sources. I think it should look fine.

[dbwiddis]

You answered what I meant by synchronize. Maven rewrites license headers if they are not the first thing in the file, so I didn't know if you wanted to enter those yourself or if this code will do the necessary order.

[cilki]

It looks like my IDE hides the comment along with the license header, which I was not expecting.

Should I instead/additionally use @Generated on each class? It's in the annotation processing package, but it's general enough to apply here too.

[dbwiddis]

Why is this list repeated 5 lines later? Is this not configurable?

[cilki]

Agreed. There are quite a few things that I need to clean up in the API generator because, as you can see, the code isn't very readable.

[dbwiddis]

Not sure yet where this is thrown but we need a layered structure where we can allow users to catch a broader list than NoSuchQueryMethod but not suppress all RuntimeExceptions. I've tried to not have OSHI throw exceptions at all and we should permit the user to catch and log most OSHI exceptions of the form, "I'm sorry, Dave. I'm afraid I can't do that."

This is okay if it's something that will be caught immediately by a developer writing bad code, however, similar to the ClassCastExceptions thrown in the WmiUtil.

[cilki]

Actually this probably needs to be changed, but I'm not sure what to do in this case. This exception is thrown if the user tries to query an attribute that has no query method configured. This can happen in two ways:

• We forgot to implement the query method or annotate it with @Query(ATTRIBUTE_NAME).
• The user tried to query an attribute that requires an external software package that wasn't found or the privilege requirements are not met

[dbwiddis]

In the first case, it's a developer thing and a Runtime Excption is ok. In the second, I don't want to choke because the user didn't install an external software package. A pleasantly polite log message suggesting they do so would be preferred.

[cilki]

In both cases, the attribute's value in the container will remain null. This is why I have all of the attribute data types set to be boxed types, but they should eventually be Optionals instead. We can decide later if we want to include the error message in the Optional by building an OshiResult class.

[dbwiddis]

This exception message should be more verbose.

Also, it's impossible for the code to ever reach here since we throw a (more general but more verbose) exception above on this case.

[dbwiddis]

Given that we throw this exception here, in a static initializer block, it's impossible for platform (should that be capitalized as a static final variable?) to be anything but one of the 5 implemented enums.

And why isn't this an "UnsupportedOperationException" like below (or better, a custom UnupportedPlatformException.)

[dbwiddis]

You should generally almost always use an ArrayList<> unless you have a very specific use case for which LinkedLists is better (e.g., a high volume of adds / removes at only the head or tail).

[cilki]

I went with LinkedList to avoid having to call trimToSize later since that list will never change after initialization. I'm actually not sure if the list is even necessary, but it's there just in case. I'll see if I can use Collections.unmodifiableList which would be even better.

[cilki]

Why gen/main/java vs. src/gen/java or src/main/gen?

There doesn't seem to be a standard so I just wanted to keep the generated sources as far away from the non-generated stuff as possible. I'm used to using gen/main/java from working with Protobuf, but I'll change it to src/gen/java (which sounds better to me anyway). IDE support should be no different between the two. We just have to configure an additional source directory in Maven.

Status indicator on query method

I also like the idea of returning a status indicator enum. I'll make it so that query methods can either be void or return the status enum.

@RequiresRoot needs a bit more fine grain

I'm glad this situation arose because it highlights the flexibility of the drivers. To solve this elegantly, you just define two query methods for the same attribute (ProcessorID). One has @RequiresRoot and fetches from the standard data source and the other is an extension query method that requires cpuid. When the driver is initialized, ComponentDriver finds both methods and checks the requirements (whether we have root access and whether cpuid is installed). Depending on the situtation, both, one, or neither of the query methods will be called when the user queries for ProcessorID. Additionally, @Fallback can optionally configure which method has priority. This situtation is probably one of the biggest strengths of my design.

While @RequiresRoot should probably be renamed something more cross-platform, I think the usage is exactly what we want.

I'm not sure what you mean by closable resources, or why PDH Counters are an exception

I think we discussed in #9 that the only resource we should hold onto are PDH counters. This would be a closable resource because the driver needs to be "closed" in order to know when to release the counters. The design doesn't incorporate try-with-resources anywhere, so Java 9's Cleaner API is probably the only way to release those resources. After looking at the API more closely, I think our use case is what the API is intended to be used for.

---

OK so we still have the problem that the compatibility of these attributes on five platforms don't map onto an inheritance hierarchy perfectly. Specifying that an attribute is compatible with a platform works fine, but specifying that an attribute is incompatible with a platform forces that attribute to become platform-specific on all of the other platforms. This isn't ideal so I also experimented with a radically different type of API.

I call it the AttributeKey model because each attribute has a public static final enum-like object that the user references when retrieving/querying an attribute. AttributeKey is generic, so you don't have to worry about type safety, but the catch is that the usage looks like this:

disk.get(DiskAttribute.READ_BYTES); // Attribute compatible with every platform
disk.query(DiskAttribute.INODES); // Linux platform-specific attribute

This eliminates all of the platform-specific container classes because the container objects basically become wrappers for a Map. It solves some problems by relaxing the API, but creates some new problems and isn't as nice to use.

[dbwiddis]

I think we discussed in #9 that the only resource we should hold onto are PDH counters.
Ah, I've since changed that during my API overhaul. I release all the PDH resources after each query. I measured the performance difference and it's negligible for periodic polling.

I even got rid of my cached CFStrings used for the macOS dictionary lookups. It's kind of a pain to recreate them (especially "constants") each query, but it avoids pointer leaks. So I really don't think I'm keeping any resources open anymore.

I like the attribute key query structure (it's similar to how I fetch results from the WMI queries using an enum) but I missed the connection between that and compatibility. Mostly, there will be things that are all-OS, or all-OS-except-one(usually Windows), or one-OS-only.

[cilki]

Here's what the container API looks like after we specify that the inode attribute (for example) is incompatible on Windows, but is compatible with basically everything else:

• Disk
  • inodes
• DiskLinux
  • inodes
• DiskMac
  • inodes
• DiskFreeBsd
  • inodes
• DiskSolaris
  • inodes
• DiskWindows

Now you have to find out what platform you're on and cast your Disk to DiskLinux (or whatever) to get the inode count. Attribute keys solves this because it's less safe during runtime. With attribute keys, you just do this:

disk.get(DiskAttribute.INODES);

If you're on one of the 4 platforms that supports inodes, then you're good.

I use the attribute key pattern in my other project to reference nodes of a hierarchical tree and it works pretty well, but I'm not convinced it's the best API for OSHI.

[cilki]

I think offering AttributeKey access in addition to the current API could work because users could then choose whether they want to be platform-safe (by calling getWindowsSystem(), etc) or platform-unsafe (by simply calling get with a platform-specific attribute key).

This adds some complexity, but I'll try to figure out if it would be worth it.