Add Wbemcli classes needed to query WMI #994

dbwiddis · 2018-08-05T18:23:55Z

No description provided.

matthiasblaesing

I did a first pass - could you please have a look at the comments?

matthiasblaesing · 2018-08-11T09:07:58Z

contrib/platform/src/com/sun/jna/platform/win32/Ole32.java

+    HRESULT CoSetProxyBlanket(Pointer pProxy, //
+            int dwAuthnSvc, //
+            int dwAuthzSvc, //
+            BSTR pServerPrincName, // OLECHAR


Beware! OLECHAR is not a BSTR. The parameter pServerPrincName is a OLECHAR array, so I'd bind this as a LPOLESTR.

MS is evil! I saw typedef OLECHAR * BSTR and thought I was good. I only ever call that function with null so this never reared its ugly head for me... will fix.

matthiasblaesing · 2018-08-11T09:15:44Z

contrib/platform/src/com/sun/jna/platform/win32/Ole32.java

+     *
+     *         E_OUT_OF_MEMORY Out of memory.
+     */
+    HRESULT CoInitializeSecurity(Pointer pSecDesc, NativeLong cAuthSvc, Pointer asAuthSvc, Pointer pReserved1,


pSecDec is a PSECURITY_DESCRIPTOR, so could WinNT.SECURITY_DESCRIPTOR be used here?

Yep. Another case where I always used null and was lazy.

matthiasblaesing · 2018-08-11T09:21:15Z

contrib/platform/src/com/sun/jna/platform/win32/Ole32.java

+     *
+     *         E_OUT_OF_MEMORY Out of memory.
+     */
+    HRESULT CoInitializeSecurity(Pointer pSecDesc, NativeLong cAuthSvc, Pointer asAuthSvc, Pointer pReserved1,


cAuthSvc is a LONG -- for the windows data types, microsoft says

LONG A 32-bit signed integer. The range is 2147483648 through 2147483647 decimal.

So int should be a save binding.

matthiasblaesing · 2018-08-11T09:30:26Z

contrib/platform/src/com/sun/jna/platform/win32/Ole32.java

+     *
+     *         E_INVALIDARG One or more arguments is invalid.
+     */
+    HRESULT CoSetProxyBlanket(Pointer pProxy, //


Should pProxy be Pointer or Unknown or IUnknown?

It's the pointed-to value of the last argument of IwbemLocator::ConnectServer which is a pointer to an IWbemServices object. In the code I use this Pointer in the IWbemServices initializer. Not sure what other mapping it should be.

matthiasblaesing · 2018-08-11T10:17:44Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+        }
+
+        public HRESULT Get(BSTR wszName, NativeLong lFlags, VARIANT.ByReference pVal, Pointer pvtType,
+                LongByReference plFlavor) {


According to the documents from microsoft this is the definition:

HRESULT Get( LPCWSTR wszName, long lFlags, VARIANT *pVal, CIMTYPE *pType, long *plFlavor );

So I would use String fro wszName and wrap it into a WString for the call. Microsoft defines the sizes of the base types in:

https://msdn.microsoft.com/de-de/library/cc953fe1.aspx

So lFlags can be bound as int and plFlavor is a IntByReference. The LongByReference is to big. pType (it is not pvtType) is defined as:

typedef long CIMTYPE;

So it is not a pointer!

Wouldn't CIMTYPE *pType mean that pType is a pointer to CIMTYPE so a Pointer is valid here (IntByReference would be better as you note for long *plFlavor). I have read the value from this call using Pointer.getInt() and it has been the CIMTYPE.

matthiasblaesing · 2018-08-11T10:23:45Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+            super(pvInstance);
+        }
+
+        public HRESULT Next(NativeLong lTimeOut, NativeLong uCount, PointerByReference ppObjects,


Same comment as in IWbemClassObject applies - is it expected, that this method is called on a non windows platform? if not, NativeLong can be substituted with int.

matthiasblaesing · 2018-08-11T10:29:04Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+        // Connect to WMI through the IWbemLocator::ConnectServer method
+        // Connect to the namespace with the current user and obtain pointer
+        // pSvc to make IWbemServices calls.
+        HRESULT hres = loc.ConnectServer(new BSTR(namespace), null, null, null, null, null, null, pSvc);


Don't use the BSTR constructor. BSTR need to be allocated via OleAuto.INSTANCE.SysAllocString and they need to be freed with OleAuto.INSTANCE.SysFreeString.

matthiasblaesing · 2018-08-11T10:32:06Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+        }
+
+        public HRESULT ConnectServer(BSTR strNetworkResource, BSTR strUser, BSTR strPassword, BSTR strLocale,
+                NativeLong lSecurityFlags, BSTR strAuthority, Pointer pCtx, PointerByReference ppNamespace) {


You could use String instead of BSTR and allocate + release the BSTRs inside the function (see below for allocation of BSTR).

For pCtx should this really be a plain Pointer?

For pCtx it would probably be an IWbemContext object. But in all the sample code I have seen on MS sites for WMI connections (both local and remote) it's been null. Reading the docs, it looks like IUnknown (or is there a ByReference version of it) may be correct?

matthiasblaesing · 2018-08-11T10:39:01Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+    /**
+     * Initializes COM library and sets security to impersonate the local user
+     */
+    public static void initCOM() {


This looks dangerous if you combine it with other COM calling code.

I agree it looks dangerous but I'm not sure how else to handle it. I can ignore "already initialized" errors but then if I uninit later, it will kill the external program's COM code. If I don't uninit, it's a resource leak.

Make the user responsible for initializing COM. Else you need to guess how the user will use it. The COM Utilities have offer both options. COMThread handles the creation of a thread for dispatching COM Threads with correctly initialized COM. Factory uses that, while ObjectFactory allows creation of call proxies without a different thread.

I found it better to handle COM initialization myself, so I prefer to use the ObjectFactory.

There is a problem with your code - when calls to the utility methods are done from different threads, COM will only be initialized once. This violates the requirement, that CoInitializeEx must be called on each thread making COM calls.

I've moved the COM initialization outside the Util class.

dbwiddis · 2018-08-12T03:43:31Z

I think I've addressed all the comments except the type mapping for the last argument of ConnectServer. It works as I have it with a Pointer. I tried various combinations of Unknown and ByReference but ended up unable to access the multiple layers of pointers that PointerByReference allows.

matthiasblaesing · 2018-08-19T18:43:40Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+     * Exception encountered in this class
+     */
+    @SuppressWarnings("serial")
+    class WbemcliException extends RuntimeException {


This is only used in the WbemcliUtil class, so I would move it there.

Actually it's used if CoCreateInstance() fails.

I see - thank you for the pointer, I really overlooked that case.

To be fair, there's an argument for just returning HRESULT and throwing the exception from the caller (in the Util method). Let me know if you'd prefer that.

COMBindingBaseObject uses COMUtils#checkRC which raises a COMException on error - that would align Wbemcli with other COM bindings.

Good timing, I was actually just about to update this. Based on the above, I'll

return null from this method if it fails

handle null in the util class with a COMException along with other uses of this exception.

matthiasblaesing · 2018-08-19T18:45:06Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+            HRESULT hres = Ole32.INSTANCE.CoCreateInstance(CLSID_WbemLocator, null, WTypes.CLSCTX_INPROC_SERVER,
+                    IID_IWbemLocator, pbr);
+            if (COMUtils.FAILED(hres)) {
+                Ole32.INSTANCE.CoUninitialize();


This looks like an artifact from the "handle COM initialization in the calls" approach. There is no reason to uninitialize COM here.

Yeah, I had already removed that, somehow I committed it to a different branch. Same with the removed variables.

matthiasblaesing · 2018-08-19T19:17:49Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+        public Integer getInteger(T property, int index) {
+            Object o = this.propertyMap.get(property).get(index);
+            if (o == null) {
+                return 0;


I'm a bit irritated here - why return a primitive 0 instead of the java null? I would understand if you return int, but with a nullable type?

That's a very good question, and I went back and forth on this implementation several times. My reasoning:

Requiring the user to null-check every return takes away some of the value of a utility class that provides sensible defaults (zero or empty string). The user still has the ability to test null (and even determine types if not-null) directly using getValue(). All the other getters are convenience methods, and I wanted them to be convenient.

The return type should be tied directly to the CIM type. getInteger() should only be used for uint16, sint32, and uint32 CIM types, all of which suggest a numeric value. The only time we run into an issue is if WMI has no value for that column, and we don't check the type.

I've also considered having different return types based on the CIM Type rather than the value type encoded in the Variant. For example, both String and uint64 return as a Variant BSTR, but the latter requires further parsing (to either long or possibly a BigInteger or WinNT.LARGE_INTEGER. All of which returns to the question of how to treat parse exceptions / nulls. I eventually settled on what you see as a happy medium.

If I had my druthers I'd probably use Java 8's Optional<Integer> here. But that's not an option (pun intended).

All these numeric values began their life as primitives, from VARIANT's various .*value() methods. They only got boxed when being added to the Lists. When I wrote this, I unboxed and returned primitives, and that's why I initially wrote the defaults.

In many cases in my application I would immediately re-box them to put into another list or map. So since the value was already boxed in the list, I felt returning it as it was (with a simple typecast) was the most direct option and changed it. So the code you see is a relic of the original return type but I still thought the defaults were more useful than null (since the user still has access to test null).

From the perspective of a user, if they want the default 0 it's just
result.getInteger(WmiProperty.WMIFIELD, 0)
If they want a different default:
result.getValue(WmiProperty.WMIFIELD, 0) == null ? -1 : result.getInteger(WmiProperty.WMIFIELD, 0)

If I were to return null instead of a guaranteed numeric it would require the user to always do:
result.getInteger(WmiProperty.WMIFIELD, 0) == null ? 0 : result.getInteger(WmiProperty.WMIFIELD, 0)

matthiasblaesing · 2018-08-19T19:22:24Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+        Runtime.getRuntime().addShutdownHook(new Thread() {
+            @Override
+            public void run() {
+                OleAuto.INSTANCE.SysFreeString(WQL);


I would not preallocated WQL and do the allocation in the query. At least I would measure if it really is a significant part of the call cost. This construction looks fragile.

The allocation and release take an average of 33 microseconds for both calls. Assuming (generously) I could kill one of them that'd save 16 microseconds per invocation. So, yeah, not too significant. :)

matthiasblaesing · 2018-08-19T19:23:07Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+
+    // Track initialization of COM and Security
+    private static boolean comInitialized = false;
+    private static boolean securityInitialized = false;


These variables are not used anymore.

matthiasblaesing · 2018-08-19T19:26:41Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+import com.sun.jna.ptr.PointerByReference;
+
+/**
+ * This header is used by Remote Desktop Services.


?! Not sure what the header wants to tell me?

Well, I stole it from here. I guess I can make it more useful.

matthiasblaesing · 2018-08-25T13:43:28Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+        /**
+         * Increment the result count by one.
+         */
+        public void incrementResultCount() {


add is private, this should also be private

matthiasblaesing · 2018-08-25T13:47:28Z

contrib/platform/src/com/sun/jna/platform/win32/WbemcliUtil.java

+        OleAuto.INSTANCE.SysFreeString(queryStr);
+        OleAuto.INSTANCE.SysFreeString(wql);
+        if (COMUtils.FAILED(hres)) {
+            svc.Release();


why is svc released here? The value is passed in, so I would keep away from handling it.

It was released there in the sample code that I ported, although since I've moved out other resource releasing, may as well do that here, as well.

matthiasblaesing · 2018-08-25T14:45:46Z

contrib/platform/src/com/sun/jna/platform/win32/Wbemcli.java

+ * A copy is also included in the downloadable source code package
+ * containing JNA, in file "AL2.0".
+ */
+package com.sun.jna.platform.win32;


These are COM bindings could you please move this and the Util class to the com.sun.jna.platform.win32.COM package?

…galArgumentException

matthiasblaesing · 2018-08-26T18:01:50Z

I would be willing to merge - shall I?

dbwiddis · 2018-08-26T18:20:30Z

Woohoo! Only one other change I was considering: The clsObj.Get() call is capable of returning the CIM type. This would allow for a more robust type checking by the user (e.g., better distinguish a String object with Variant type VT_BSTR between the various DATETIME, UINT64, and STRING, and other options: see left hand column of line 432 of the util class). While it's not really needed (the caller should already know the type from the WMI table documentation) I think it's useful to include.

Thanks as always for the educational code review and your patience.

dbwiddis · 2018-08-26T20:19:02Z

OK, ready to merge! If you like my latest commit, include it. If not, back out the last commit and merge!

matthiasblaesing · 2018-09-03T20:17:00Z

Hi Daniel, I had another look at this (sorry for the delay). Looking at the Wbem classes, they look a lot like JDBC and could be wrapped similarly, I did not pursue that further, as that might be overkill. However, I added a few changes, that you might want to consider:

https://github.com/matthiasblaesing/jna/commits/dbwiddis-wmi

Please have a look at the last three commits of the branch referenced above (If all went well, you should be able to pull directly from it). The first commit adds a bit binding sugar, the second moves WmiQuery more into a object oriented direction and the third fixes the unittests.

dbwiddis · 2018-09-04T00:22:16Z

Looks great! I've added your commits to my PR. I submitted one final commit which removes an unused variable and unneeded cast, some unneeded javadoc params , and adds to the Wbemcli class javadocs to better match the MSDN documentation. (And my formatter spaces slightly different than yours.) Tests fine on my system, good to go here!

matthiasblaesing · 2018-09-06T13:02:45Z

Thank you for your work - its appreciated. I added a slight update:

matthiasblaesing@68c8a96

I squashed the work into one feature commit and merged that to master.

dbwiddis force-pushed the wmi branch 2 times, most recently from 355b21e to 3afe6ac Compare August 5, 2018 18:44

Add Wbemcli classes needed to query WMI

d6f5dcf

dbwiddis force-pushed the wmi branch from 3afe6ac to d6f5dcf Compare August 5, 2018 20:37

matthiasblaesing reviewed Aug 11, 2018

View reviewed changes

Fix type mappings and BSTR allocation

60ac5be

dbwiddis added 2 commits August 18, 2018 22:53

Remove COM initialization from Util

b0a99f2

Merge branch 'master' into wmi

c00cfef

matthiasblaesing reviewed Aug 19, 2018

View reviewed changes

Remove unused variables, other code review updates

ba40433

dbwiddis force-pushed the wmi branch from 706981f to ba40433 Compare August 20, 2018 01:18

matthiasblaesing reviewed Aug 25, 2018

View reviewed changes

dbwiddis added 3 commits August 25, 2018 11:50

Move Wbemcli classes to c.s.j.p.win32.COM

052b111

Change WbemcliExceptions to COMException, ClassCastException, or Ille…

3fe60dc

…galArgumentException

Code review: visibility, resources on exception

f34e7a4

dbwiddis force-pushed the wmi branch 2 times, most recently from c5a6ff3 to 8b5c3a3 Compare August 26, 2018 04:59

Remove convenience return types from WmiResult

777644a

dbwiddis force-pushed the wmi branch from 8b5c3a3 to 777644a Compare August 26, 2018 05:00

Include CIM Type in WMI results

b549fda

dbwiddis force-pushed the wmi branch from 3c79893 to b549fda Compare August 26, 2018 20:13

matthiasblaesing added 3 commits September 3, 2018 21:59

Move more Java Type Mapping into the bindings

4f88054

Move WbemcliUtil to a more object oriented style

062ea55

Fix unittest for WbemcliTest

b256c76

dbwiddis force-pushed the wmi branch from bb0b15e to 192836a Compare September 4, 2018 00:19

Unneeded variable, cast, and some docs

f0e50d4

dbwiddis force-pushed the wmi branch from 192836a to f0e50d4 Compare September 4, 2018 01:27

matthiasblaesing closed this in 683fd9b Sep 6, 2018

dbwiddis deleted the wmi branch February 18, 2019 02:46

Add Wbemcli classes needed to query WMI #994

Add Wbemcli classes needed to query WMI #994

Conversation

dbwiddis commented Aug 5, 2018

matthiasblaesing 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

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

dbwiddis Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

dbwiddis commented Aug 12, 2018

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

dbwiddis Aug 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

matthiasblaesing commented Aug 26, 2018

dbwiddis commented Aug 26, 2018

dbwiddis commented Aug 26, 2018

matthiasblaesing commented Sep 3, 2018

dbwiddis commented Sep 4, 2018 • edited Loading

matthiasblaesing commented Sep 6, 2018

dbwiddis Aug 19, 2018 •

edited

Loading

dbwiddis Aug 19, 2018 •

edited

Loading

dbwiddis commented Sep 4, 2018 •

edited

Loading