Fixed slow command line retrieval on Windows #862
Conversation
There was a problem hiding this comment.
Hi, really impressive work, I spent days on this problem trying to port psutil C code over in gopsutil. We will happily close this long standing issue #250.
I have only some stylistic remarks and issues with too broad exported API in the PR that I commented in this review.
Also, I was able to test on win7 and win10 x64, baremetal and VM (respectively), but not on some customized x32 win7 VM called "Tiny7", with every calls to proc.Cmdline() resulting with an could not get CommandLine: cannot locate process PEB error, but it could be because this VM is heavily trimmed down. I would like to see it work on a 32bit Windows, did you test these changes on one?
|
Hi @Lomanic , great work with this project. I'll review the changes and commit them. Kind regards. |
|
Fixed the issue on 32-bit OSes, order of imports and public/private access to structs. |
| @@ -49,21 +49,33 @@ const ( | |||
| PDH_NO_DATA = 0x800007d5 | |||
| ) | |||
|
|
|||
| const ( | |||
| ProcessBasicInformation = 0 | |||
| ProcessWow64Information = 26 | |||
There was a problem hiding this comment.
these two constants are only used in the process package, there is no point in putting them in the common package
| PdhGetFormattedCounterValue = ModPdh.NewProc("PdhGetFormattedCounterValue") | ||
| PdhCloseQuery = ModPdh.NewProc("PdhCloseQuery") | ||
|
|
||
| procQueryDosDeviceW = Modkernel32.NewProc("QueryDosDeviceW") |
There was a problem hiding this comment.
don't go against go-fmt, this is a lost battle ;)
| fallthrough | ||
| case PROCESSOR_ARCHITECTURE_IA64: | ||
| fallthrough | ||
| case PROCESSOR_ARCHITECTURE_AMD64: |
There was a problem hiding this comment.
would be more idiomatic to use case PROCESSOR_ARCHITECTURE_ARM64, PROCESSOR_ARCHITECTURE_IA64, PROCESSOR_ARCHITECTURE_AMD64 (same for the 32 bit case above), fallthrough is really rarely used in Go
| //we are on a 32-bit process reading an external 32-bit process | ||
| var info processBasicInformation32 | ||
|
|
||
| ret, _, _ := common.ProcNtQueryInformationProcess.Call( |
There was a problem hiding this comment.
queryPebAddress should probably return both the address and the possible error returned from calls to win32 API in case they fail, getProcessCommandLine would return this error if err != nil (that's the usual idiom in Go).
| ProcGetSystemTimes = Modkernel32.NewProc("GetSystemTimes") | ||
| ProcNtQuerySystemInformation = ModNt.NewProc("NtQuerySystemInformation") | ||
| ProcRtlGetNativeSystemInformation = ModNt.NewProc("RtlGetNativeSystemInformation") | ||
| ProcRtlNtStatusToDosError = ModNt.NewProc("RtlNtStatusToDosError") |
There was a problem hiding this comment.
RtlGetNativeSystemInformation and RtlNtStatusToDosError are both unused, please remove them
Hi,
This patch aims to fix the slow command line retrieval on Windows described in issue #250
The code can also read 64-bit processes from 32-bit ones.
It removes WMI usage by the faster ReadProcessMemory (actually ntdll apis are used).
Kind regards,
Mauro.