Make DriverCertification MSI property public
PATCH 1/4 Make DriverCertification MSI property public
Making DriverCertification public (containing only upper-case letters) allows the property set by FindSystemInfo custom action to be passed from InstallUISequence sequence to InstallExecuteSequence. This eliminates the need to execute FindSystemInfo in both sequences.
--- src/openvpnmsica/openvpnmsica.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
src/openvpnmsica/openvpnmsica.c +2 -2
@@ -289,11 +289,11 @@ FindSystemInfo(_In_ MSIHANDLE hInstall) } } - uiResult =MsiSetProperty (hInstall,TEXT ("DriverCertification" ), ver_info.dwMajorVersion >=10 ? ver_info.wProductType >VER_NT_WORKSTATION ?TEXT ("whql" ) :TEXT ("attsgn" ) :TEXT ("" )); + uiResult =MsiSetProperty (hInstall,TEXT ("DRIVERCERTIFICATION" ), ver_info.dwMajorVersion >=10 ? ver_info.wProductType >VER_NT_WORKSTATION ?TEXT ("whql" ) :TEXT ("attsgn" ) :TEXT ("" ));if (uiResult !=ERROR_SUCCESS ) {SetLastError (uiResult);/* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */ -msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"TAPINTERFACES\") failed" , __FUNCTION__); +msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"DRIVERCERTIFICATION\") failed" , __FUNCTION__); goto cleanup_CoInitialize; }
Signature · 1 line
2.19.0.windows.1
PATCH 2/4 Extend FindSystemInfo custom action to detect OpenVPNService state
--- src/openvpnmsica/openvpnmsica.c | 168 ++++++++++++++++++++++++++++---- 1 file changed, 151 insertions(+), 17 deletions(-)
src/openvpnmsica/openvpnmsica.c +151 -17
@@ -44,6 +44,7 @@#include <tchar.h> #ifdef _MSC_VER +#pragma comment(lib, "advapi32.lib")#pragma comment(lib, "iphlpapi.lib")#pragma comment(lib, "shell32.lib")#pragma comment(lib, "shlwapi.lib")
@@ -193,29 +194,34 @@ _openvpnmsica_debug_popup(_In_z_ LPCTSTR szFunctionName)#endif -UINT __stdcall -FindSystemInfo(_In_ MSIHANDLE hInstall) +/** +* Detects Windows version and sets DRIVERCERTIFICATION property to "", "whql", or "attsgn" +* accordingly. +* +* @param hInstall Handle to the installation provided to the DLL custom action +* +* @return ERROR_SUCCESS on success; An error code otherwise +* See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx +*/ +static UINT +openvpnmsica_set_driver_certification(_In_ MSIHANDLE hInstall) { -#ifdef _MSC_VER -#pragma comment(linker, DLLEXP_EXPORT) -#endif - -openvpnmsica_debug_popup (TEXT (__FUNCTION__)); -UINT uiResult; -BOOL bIsCoInitialized =SUCCEEDED (CoInitialize (NULL )); - -/* Set MSI session handle in TLS. */ -struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue (openvpnmsica_tlsidx_session); - s->hInstall = hInstall;/* Get Windows version. */ +#ifdef _MSC_VER +#pragma warning(push) +#pragma warning(disable: 4996)/* 'GetVersionExW': was declared deprecated. */ +#endif OSVERSIONINFOEX ver_info = { .dwOSVersionInfoSize =sizeof (OSVERSIONINFOEX ) };if (!GetVersionEx ((LPOSVERSIONINFO )&ver_info)) { uiResult =GetLastError ();msg (M_NONFATAL |M_ERRNO ,"%s: GetVersionEx() failed" , __FUNCTION__); - goto cleanup_CoInitialize; +return uiResult; } +#ifdef _MSC_VER +#pragma warning(pop) +#endif /* The Windows version is usually spoofed, check using RtlGetVersion(). */ TCHAR szDllPath[0x1000 ];
@@ -294,17 +300,145 @@ FindSystemInfo(_In_ MSIHANDLE hInstall) {SetLastError (uiResult);/* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */ msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"DRIVERCERTIFICATION\") failed" , __FUNCTION__); - goto cleanup_CoInitialize; +return uiResult; + } + +return ERROR_SUCCESS ; +} + + +/** +* Detects if the OpenVPNService service is in use (running or paused) and sets +* OPENVPNSERVICE to the service process PID, or its path if it is set to +* auto-start, but not running. +* +* @param hInstall Handle to the installation provided to the DLL custom action +* +* @return ERROR_SUCCESS on success; An error code otherwise +* See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx +*/ +static UINT +openvpnmsica_set_openvpnserv_state(_In_ MSIHANDLE hInstall) +{ +UINT uiResult; + +/* Get Service Control Manager handle. */ +SC_HANDLE hSCManager =OpenSCManager (NULL ,SERVICES_ACTIVE_DATABASE ,SC_MANAGER_CONNECT ); +if (hSCManager ==NULL ) + { + uiResult =GetLastError (); +msg (M_NONFATAL |M_ERRNO ,"%s: OpenSCManager() failed" , __FUNCTION__); +return uiResult; + } + +/* Get OpenVPNService service handle. */ +SC_HANDLE hService =OpenService (hSCManager,TEXT ("OpenVPNService" ),SERVICE_QUERY_STATUS |SERVICE_QUERY_CONFIG ); +if (hService ==NULL ) + { + uiResult =GetLastError (); +if (uiResult ==ERROR_SERVICE_DOES_NOT_EXIST ) { +/* This is not actually an error. */ + goto cleanup_OpenSCManager; + } +msg (M_NONFATAL |M_ERRNO ,"%s: OpenService(\"OpenVPNService\") failed" , __FUNCTION__); + goto cleanup_OpenSCManager; + } + +/* Query service status. */ +SERVICE_STATUS_PROCESS ssp; +DWORD dwBufSize; +if (!QueryServiceStatusEx (hService,SC_STATUS_PROCESS_INFO , (LPBYTE )&ssp,sizeof (ssp), &dwBufSize)) + { + uiResult =GetLastError (); +msg (M_NONFATAL |M_ERRNO ,"%s: QueryServiceStatusEx(\"OpenVPNService\") failed" , __FUNCTION__); + goto finish_QueryServiceStatusEx; + } + +switch (ssp.dwCurrentState) + { +case SERVICE_START_PENDING : +case SERVICE_RUNNING : +case SERVICE_STOP_PENDING : +case SERVICE_PAUSE_PENDING : +case SERVICE_PAUSED : +case SERVICE_CONTINUE_PENDING : + { +/* Set OPENVPNSERVICE property to service PID. */ +TCHAR szPID[10 /*MAXDWORD in decimal*/ +1 /*terminator*/ ]; +_stprintf_s ( + szPID,_countof (szPID), +TEXT ("%u" ), + ssp.dwProcessId); + + uiResult =MsiSetProperty (hInstall,TEXT ("OPENVPNSERVICE" ), szPID); +if (uiResult !=ERROR_SUCCESS ) + { +SetLastError (uiResult);/* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */ +msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"OPENVPNSERVICE\") failed" , __FUNCTION__); + } + goto cleanup_OpenService; + } +break ; + } +finish_QueryServiceStatusEx:; + +// Service is not started. Is it set to auto-start? +// MSDN describes the maximum buffer size for QueryServiceConfig() to be 8kB. +// This is small enough to fit on stack. +BYTE _buffer_8k[8192 ]; +LPQUERY_SERVICE_CONFIG pQsc = (LPQUERY_SERVICE_CONFIG )_buffer_8k; + dwBufSize =sizeof (_buffer_8k); +if (!QueryServiceConfig (hService, pQsc, dwBufSize, &dwBufSize)) + { + uiResult =GetLastError (); +msg (M_NONFATAL |M_ERRNO ,"%s: QueryServiceStatusEx(\"QueryServiceConfig\") failed" , __FUNCTION__); + goto cleanup_OpenService; + } + +if (pQsc->dwStartType <=SERVICE_AUTO_START ) + { + uiResult =MsiSetProperty (hInstall,TEXT ("OPENVPNSERVICE" ), pQsc->lpBinaryPathName); +if (uiResult !=ERROR_SUCCESS ) + { +SetLastError (uiResult);/* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */ +msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"OPENVPNSERVICE\") failed" , __FUNCTION__); + goto cleanup_OpenService; + } } uiResult =ERROR_SUCCESS ; -cleanup_CoInitialize: -if (bIsCoInitialized)CoUninitialize (); +cleanup_OpenService: +CloseServiceHandle (hService); +cleanup_OpenSCManager: +CloseServiceHandle (hSCManager);return uiResult; } +UINT __stdcall +FindSystemInfo(_In_ MSIHANDLE hInstall) +{ +#ifdef _MSC_VER +#pragma comment(linker, DLLEXP_EXPORT) +#endif + +openvpnmsica_debug_popup (TEXT (__FUNCTION__)); + +BOOL bIsCoInitialized =SUCCEEDED (CoInitialize (NULL )); + +/* Set MSI session handle in TLS. */ +struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue (openvpnmsica_tlsidx_session); + s->hInstall = hInstall; + +openvpnmsica_set_driver_certification (hInstall); +openvpnmsica_set_openvpnserv_state (hInstall); + +if (bIsCoInitialized)CoUninitialize (); +return ERROR_SUCCESS ; +} + +UINT __stdcall FindTAPInterfaces(_In_ MSIHANDLE hInstall) {
Signature · 1 line
2.19.0.windows.1
PATCH 3/4 Uncrustify tapctl and openvpnmsica
In reply to Simon Rozman--- src/openvpnmsica/dllmain.c | 33 +-- src/openvpnmsica/msica_op.c | 362 +++++++++++++++++------------- src/openvpnmsica/msica_op.h | 114 +++++----- src/openvpnmsica/msiex.c | 51 +++-- src/openvpnmsica/msiex.h | 86 ++++---- src/openvpnmsica/openvpnmsica.c | 292 ++++++++++++++++--------- src/openvpnmsica/openvpnmsica.h | 10 +- src/tapctl/basic.h | 2 +- src/tapctl/error.h | 3 +- src/tapctl/main.c | 60 +++-- src/tapctl/tap.c | 375 ++++++++++++++++++-------------- src/tapctl/tap.h | 22 +- 12 files changed, 827 insertions(+), 583 deletions(-)
src/openvpnmsica/dllmain.c +19 -14
@@ -42,10 +42,11 @@ DWORD openvpnmsica_tlsidx_session = TLS_OUT_OF_INDEXES;/** * DLL entry point */ -BOOL WINAPI DllMain( +BOOL WINAPI +DllMain ( _In_ HINSTANCE hinstDLL, - _In_ DWORD dwReason, - _In_ LPVOID lpReserved) + _In_ DWORD dwReason, + _In_ LPVOID lpReserved) {UNREFERENCED_PARAMETER (hinstDLL);UNREFERENCED_PARAMETER (lpReserved);
@@ -56,13 +57,15 @@ BOOL WINAPI DllMain(/* Allocate TLS index. */ openvpnmsica_tlsidx_session =TlsAlloc ();if (openvpnmsica_tlsidx_session ==TLS_OUT_OF_INDEXES ) + {return FALSE; -/* Fall through. */ + } +/* Fall through. */ case DLL_THREAD_ATTACH : {/* Create TLS data. */ -struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)malloc (sizeof (struct openvpnmsica_tls_data )); +struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)malloc (sizeof (struct openvpnmsica_tls_data ));memset (s,0 ,sizeof (struct openvpnmsica_tls_data ));TlsSetValue (openvpnmsica_tlsidx_session, s);break ;
@@ -124,7 +127,7 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)else {/* Allocate on heap and retry. */ -char *szMessage = (char *)malloc (++iResultLen *sizeof (char )); +char *szMessage = (char *)malloc (++iResultLen *sizeof (char ));vsnprintf (szMessage, iResultLen, format, arglist);MsiRecordSetStringA (hRecordProg,2 , szMessage);free (szMessage);
@@ -147,21 +150,23 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)/* Field 4: The Windows error description. */ LPTSTR szErrMessage =NULL ;if (FormatMessage ( -FORMAT_MESSAGE_FROM_SYSTEM |FORMAT_MESSAGE_ALLOCATE_BUFFER |FORMAT_MESSAGE_IGNORE_INSERTS , -0 , - dwResult, -0 , - (LPTSTR )&szErrMessage, -0 , -NULL ) && szErrMessage) +FORMAT_MESSAGE_FROM_SYSTEM |FORMAT_MESSAGE_ALLOCATE_BUFFER |FORMAT_MESSAGE_IGNORE_INSERTS , +0 , + dwResult, +0 , + (LPTSTR )&szErrMessage, +0 , +NULL ) && szErrMessage) {/* Trim trailing whitespace. Set terminator after the last non-whitespace character. This prevents excessive trailing line breaks. */ -for (size_t i =0 , i_last =0 ; ; i++) +for (size_t i =0 , i_last =0 ;; i++) {if (szErrMessage[i]) {if (!_istspace (szErrMessage[i])) + { i_last = i +1 ; + } }else {
src/openvpnmsica/msica_op.c +215 -147
756 lines not on this page · raw message
src/openvpnmsica/msica_op.h +57 -57
203 lines not on this page · raw message
src/openvpnmsica/msiex.c +37 -14
118 lines not on this page · raw message
src/openvpnmsica/msiex.h +43 -43
106 lines not on this page · raw message
src/openvpnmsica/openvpnmsica.c +186 -106
678 lines not on this page · raw message
src/openvpnmsica/openvpnmsica.h +5 -5
@@ -25,9 +25,9 @@#include "../tapctl/basic.h" -/* -* Error codes (next unused 2552L) -*/ +/* +* Error codes (next unused 2552L) +*/ #define ERROR_MSICA 2550L#define ERROR_MSICA_ERRNO 2551L
@@ -59,7 +59,7 @@ extern "C" {#define DLLEXP_DECL __declspec(dllexport)#else #define DLLEXP_DECL -#define DLLEXP_EXPORT "/EXPORT:"__FUNCTION__"="__FUNCDNAME__ +#define DLLEXP_EXPORT "/EXPORT:" __FUNCTION__ "=" __FUNCDNAME__#endif
@@ -144,4 +144,4 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall); }#endif -#endif +#endif /* ifndef MSICA_H */
src/tapctl/basic.h +1 -1
@@ -51,4 +51,4 @@#define _Out_z_cap_ (n)#endif -#endif +#endif /* ifndef BASIC_H */
src/tapctl/error.h +2 -1
@@ -75,6 +75,7 @@ bool dont_mute(unsigned int flags);#endif void x_msg (const unsigned int flags,const char *format, ...);/* should be called via msg above */ +void x_msg_va (const unsigned int flags,const char *format,va_list arglist);/* Inline functions */
@@ -92,4 +93,4 @@ msg_test(unsigned int flags) return check_debug_level(flags) && dont_mute(flags); } -#endif +#endif /* ifndef ERROR_H */
src/tapctl/main.c +42 -18
170 lines not on this page · raw message
src/tapctl/tap.c +209 -166
738 lines not on this page · raw message
src/tapctl/tap.h +11 -11
@@ -47,10 +47,10 @@**/ DWORD tap_create_interface ( - _In_opt_ HWND hwndParent, + _In_opt_ HWND hwndParent, _In_opt_ LPCTSTR szDeviceDescription, - _Inout_ LPBOOL pbRebootRequired, - _Out_ LPGUID pguidInterface); + _Inout_ LPBOOL pbRebootRequired, + _Out_ LPGUID pguidInterface);/**
@@ -73,9 +73,9 @@ tap_create_interface(**/ DWORD tap_delete_interface ( - _In_opt_ HWND hwndParent, - _In_ LPCGUID pguidInterface, - _Inout_ LPBOOL pbRebootRequired); + _In_opt_ HWND hwndParent, + _In_ LPCGUID pguidInterface, + _Inout_ LPBOOL pbRebootRequired);/**
@@ -98,7 +98,7 @@ tap_set_interface_name(*/ struct tap_interface_node { -GUID guid;/** Interface GUID */ +GUID guid;/** Interface GUID */ LPTSTR szzHardwareIDs;/** Device hardware ID(s) */ LPTSTR szName;/** Interface name */
@@ -116,15 +116,15 @@ struct tap_interface_node* hwndParent to NULL. * * @param ppInterfaceList A pointer to the list to receive pointer to the first interface in -* the list. After the list is no longer required, free it using +* the list. After the list is no longer required, free it using * tap_free_interface_list(). * * @return ERROR_SUCCESS on success; Win32 error code otherwise */ DWORD tap_list_interfaces ( - _In_opt_ HWND hwndParent, - _Out_ struct tap_interface_node **ppInterfaceList); + _In_opt_ HWND hwndParent, + _Out_ struct tap_interface_node **ppInterfaceList);/**
@@ -136,4 +136,4 @@ voidtap_free_interface_list ( _In_ struct tap_interface_node *pInterfaceList); -#endif +#endif /* ifndef TAP_H */
Signature · 1 line
2.19.0.windows.1
PATCH 4/4 Detect missing TAP driver and bail out gracefully
In reply to Simon RozmanWhen no TUN/TAP driver is installed a interface cannot be created. This patch detects this condition and bails out with an error message.
This also fixes a typo in one of the error messages.
--- src/tapctl/tap.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
src/tapctl/tap.c +8 -1
@@ -460,7 +460,7 @@ tap_create_interface( &devinfo_data)) { dwResult =GetLastError (); -msg (M_NONFATAL ,"%s: SetupDiClassNameFromGuid failed" , __FUNCTION__); +msg (M_NONFATAL ,"%s: SetupDiCreateDeviceInfo failed" , __FUNCTION__); goto cleanup_hDevInfoList; }
@@ -596,6 +596,13 @@ tap_create_interface(free (drvinfo_detail_data); } +if (dwlDriverVersion ==0 ) + { + dwResult =ERROR_NOT_FOUND ; +msg (M_NONFATAL ,"%s: No driver for device \"%" PRIsLPTSTR"\" installed." , __FUNCTION__, szzHardwareIDs); + goto cleanup_DriverInfoList; + } +/* Call appropriate class installer. */ if (!SetupDiCallClassInstaller( DIF_REGISTERDEVICE,
Signature · 1 line
2.19.0.windows.1
Hi,
Quoted 8 lines · Simon Rozman
Making DriverCertification public (containing only upper-case letters) allows the property set by FindSystemInfo custom action to be passed from InstallUISequence sequence to InstallExecuteSequence. This eliminates the need to execute FindSystemInfo in both sequences.
--- src/openvpnmsica/openvpnmsica.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
It seems I'm behind times and cant figure out where these patches apply. Are these for openvpn, openvpn-build or something else?
Thanks,
Selva
Hi,
Quoted 2 lines
It seems I'm behind times and cant figure out where these patches apply. Are these for openvpn, openvpn-build or something else?
They are for openvpn repo. The MSI custom action DLL and the tapctl.exe utility were packed inside the openvpn repo to join the same workflow as openvpn.exe and other binaries (compile using openvpn-build, digitally sign, etc.).
Regards,
Simon
Hi,
Quoted 2 lines · Selva Nair
It seems I'm behind times and cant figure out where these patches apply. Are these for openvpn, openvpn-build or something else?
openvpn, but it needs all the other Simon patches applied first. Which there are lots of... I'll see that I can make some progress here over the next days.
gert
Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany [email protected]...Hi,
Quoted 10 lines · Gert Doering
Hi,
Quoted 2 lines · Selva Nair
It seems I'm behind times and cant figure out where these patches apply. Are these for openvpn, openvpn-build or something else?
openvpn, but it needs all the other Simon patches applied first. Which there are lots of... I'll see that I can make some progress here over the next days.
Oh, thanks. That explains it.
Selva
To be honest, I have no idea what this does. But I don't need to :-) - it's infrastructure for stuff you need and nobody else depends on, and the code change itself isn't introducing any new code constructs that might bring dangers. And it fixes a bug in the error reporting!
Compile tested on Ubuntu 16.04/mingw.
Your patch has been applied to the master branch.
commit fc2e28badf3736114bbc427f241961664235165c
Author: Simon Rozman Date: Wed Dec 19 21:26:08 2018 +0100
Make DriverCertification MSI property public
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg18037.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
[Openvpn-devel] [PATCH applied] Re: Extend FindSystemInfo custom action to detect OpenVPNService state
In reply to Simon RozmanThis is code I can even understand (to some extent) ;-) - overall it looks good, but a few style warts sneaked in...
+finish_QueryServiceStatusEx:;
+
+ // Service is not started. Is it set to auto-start? + // MSDN describes the maximum buffer size for QueryServiceConfig() to be 8kB. + // This is small enough to fit on stack.
.. there shouldn't be a ";" after a label, and no C++ comments... but I assume the uncrustify patch coming next will fix that...
Something else might not be obvious to the reader:
+ if (pQsc->dwStartType <= SERVICE_AUTO_START)
+ {
so what does "lesser than AUTO_START" mean? manual start? no start at all? Comparing for "lesser or equal" with something enum-like might warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */ or so... (yes, I can google this, but still).
Compile tested on Ubuntu 16.04 / mingw.
Your patch has been applied to the master branch.
commit 8148ee9d01de75d0302f224ef499eddfabc6fee2
Author: Simon Rozman Date: Wed Dec 19 21:26:09 2018 +0100
Extend FindSystemInfo custom action to detect OpenVPNService state
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg18039.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
[Openvpn-devel] [PATCH applied] Re: Uncrustify tapctl and openvpnmsica
In reply to Simon RozmanThanks :-) (while I sometimes can't make up my mind whether uncrustify makes the mode more ugly or less, at least the extra braces { ... } really help make the code more explicit in some places - even if it eats up screen real estate).
Interesting enough, uncrustify doesn't care about "label with;", and also doesn't wrap long comment lines. It *does* convert C++ comments into C, which is nice :-)
Glanced over the changes and test compiled on Ubuntu 16.04 / mingw.
Your patch has been applied to the master branch.
commit 6280d3d5536174934ee22d3840457d61896e0e3a
Author: Simon Rozman Date: Wed Dec 19 21:26:10 2018 +0100
Uncrustify tapctl and openvpnmsica
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/search?l=mid&[email protected]...
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
Re: [Openvpn-devel] [PATCH applied] Re: Extend FindSystemInfo custom action to detect OpenVPNService state
In reply to Gert DoeringHi,
Quoted 9 lines
src/openvpnmsica/openvpnmsica.c +4
-0+finish_QueryServiceStatusEx:; + +// Service is not started. Is it set to auto-start? +// MSDN describes the maximum buffer size for QueryServiceConfig() to be 8kB.
+ // This is small enough to fit on stack... there shouldn't be a ";" after a label, and no C++ comments... but I assume the uncrustify patch coming next will fix that...
The sentence following the finish_QueryServiceStatusEx label is a local variable declaration. While MSVC and MinGW are fine with or without semicolon, the Visual Studio 2017 IDE's IntelliSense is complaining with "E1072: a declaration cannot have a label".
Now, I could leave that semicolon in place to keep IntelliSense false warning away, but looking at the code I asked myself a question: "If my code is so convoluted it produces a false warning to a bot, how convoluted it must be for a human to understand?" False warnings are usually sign of overcomplex or atypical code design. I have refactored the code.
Quoted 10 lines
Something else might not be obvious to the reader:
(quoted diff) +2
-0+ if (pQsc->dwStartType <= SERVICE_AUTO_START) + {so what does "lesser than
AUTO_START" mean? manual start? no start at all? Comparing for "lesser or equal" with something enum-like might warrant a comment /*BOOT_START= 0,SYSTEM_START= 1,AUTO_START= 2 */ or so... (yes, I can google this, but still).
This is a very generic test "Is driver or service set to start automatically in any phase of the system launch?" on the Windows. I've added a comment with a brief explanation.
A patch will follow.
Regards,
Simon
Re: [Openvpn-devel] [PATCH applied] Re: Extend FindSystemInfo custom action to detect OpenVPNService state
Hi,
Quoted 5 lines · Simon Rozman
Now, I could leave that semicolon in place to keep IntelliSense false warning away, but looking at the code I asked myself a question: "If my code is so convoluted it produces a false warning to a bot, how convoluted it must be for a human to understand?" False warnings are usually sign of overcomplex or atypical code design. I have refactored the code.
Thanks for the explanation, I wasn't aware that there is a special case where a semicolon is required after a label. Even if only MSVC :-)
But second half is even better ;-)
Quoted 5 lines
Quoted 5 lines
Something else might not be obvious to the reader:
(quoted diff) +2
-0+ if (pQsc->dwStartType <= SERVICE_AUTO_START) + {
Quoted 7 lines
Quoted 4 lines
so what does "lesser than
AUTO_START" mean? manual start? no start at all? Comparing for "lesser or equal" with something enum-like might warrant a comment /*BOOT_START= 0,SYSTEM_START= 1,AUTO_START= 2 */ or so... (yes, I can google this, but still).This is a very generic test "Is driver or service set to start automatically in any phase of the system launch?" on the Windows.
Ah. So "anyone who has any clue about windows services would know and understand" - good enough. We need more clueful windows people to review windows patches :-)
(Selva is hiding somewhere)
Quoted 3 lines
I've added a comment with a brief explanation.
A patch will follow.
Thanks!
gert
Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany [email protected]...[Openvpn-devel] [PATCH applied] Re: Detect missing TAP driver and bail out gracefully
In reply to Simon RozmanLooks reasonable. Plus, correct error message :-)
Your patch has been applied to the master branch.
commit 91bc1212b4b79ac9e2cbf6d345db5df716c42a5b
Author: Simon Rozman Date: Wed Dec 19 21:26:11 2018 +0100
Detect missing TAP driver and bail out gracefully
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg18038.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering
PATCH 1/1 Refactor OpenVPNService state detection code
In reply to Gert DoeringThe code was standardized to avoid "E1072: a declaration cannot have a label" warning of Visual Studio 2017 IntelliSense.
Furthermore, a comment explaining what dwStartType <= SERVICE_AUTO_START condition is about.
This patch follows Gert's recommendations from [openvpn-devel].
Message-ID: <[email protected]...>
--- src/openvpnmsica/openvpnmsica.c | 59 +++++++++++++++++---------------- 1 file changed, 31 insertions(+), 28 deletions(-)
src/openvpnmsica/openvpnmsica.c +31 -28
@@ -365,40 +365,42 @@ openvpnmsica_set_openvpnserv_state(_In_ MSIHANDLE hInstall)/* Query service status. */ SERVICE_STATUS_PROCESS ssp;DWORD dwBufSize; -if (!QueryServiceStatusEx (hService,SC_STATUS_PROCESS_INFO , (LPBYTE )&ssp,sizeof (ssp), &dwBufSize)) +if (QueryServiceStatusEx (hService,SC_STATUS_PROCESS_INFO , (LPBYTE )&ssp,sizeof (ssp), &dwBufSize)) { - uiResult =GetLastError (); -msg (M_NONFATAL |M_ERRNO ,"%s: QueryServiceStatusEx(\"OpenVPNService\") failed" , __FUNCTION__); - goto finish_QueryServiceStatusEx; - } - -switch (ssp.dwCurrentState) - { -case SERVICE_START_PENDING : -case SERVICE_RUNNING : -case SERVICE_STOP_PENDING : -case SERVICE_PAUSE_PENDING : -case SERVICE_PAUSED : -case SERVICE_CONTINUE_PENDING : +switch (ssp.dwCurrentState) { -/* Set OPENVPNSERVICE property to service PID. */ -TCHAR szPID[10 /*MAXDWORD in decimal*/ +1 /*terminator*/ ]; -_stprintf_s ( - szPID,_countof (szPID), -TEXT ("%u" ), - ssp.dwProcessId); - - uiResult =MsiSetProperty (hInstall,TEXT ("OPENVPNSERVICE" ), szPID); -if (uiResult !=ERROR_SUCCESS ) +case SERVICE_START_PENDING : +case SERVICE_RUNNING : +case SERVICE_STOP_PENDING : +case SERVICE_PAUSE_PENDING : +case SERVICE_PAUSED : +case SERVICE_CONTINUE_PENDING : { -SetLastError (uiResult);/* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */ -msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"OPENVPNSERVICE\") failed" , __FUNCTION__); +/* Service is started (kind of). Set OPENVPNSERVICE property to service PID. */ +TCHAR szPID[10 /*MAXDWORD in decimal*/ +1 /*terminator*/ ]; +_stprintf_s ( + szPID,_countof (szPID), +TEXT ("%u" ), + ssp.dwProcessId); + + uiResult =MsiSetProperty (hInstall,TEXT ("OPENVPNSERVICE" ), szPID); +if (uiResult !=ERROR_SUCCESS ) + { +SetLastError (uiResult);/* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */ +msg (M_NONFATAL |M_ERRNO ,"%s: MsiSetProperty(\"OPENVPNSERVICE\") failed" , __FUNCTION__); + } + +/* We know user is using the service. Skip auto-start setting check. */ + goto cleanup_OpenService; } - goto cleanup_OpenService; +break ; } -break ; } -finish_QueryServiceStatusEx:; +else + { + uiResult =GetLastError (); +msg (M_NONFATAL |M_ERRNO ,"%s: QueryServiceStatusEx(\"OpenVPNService\") failed" , __FUNCTION__); + }/* Service is not started. Is it set to auto-start? */ /* MSDN describes the maximum buffer size for QueryServiceConfig() to be 8kB. */
@@ -415,6 +417,7 @@ finish_QueryServiceStatusEx:;if (pQsc->dwStartType <=SERVICE_AUTO_START ) { +/* Service is set to auto-start. Set OPENVPNSERVICE property to its path. */ uiResult =MsiSetProperty (hInstall,TEXT ("OPENVPNSERVICE" ), pQsc->lpBinaryPathName);if (uiResult !=ERROR_SUCCESS ) {
Signature · 1 line
2.19.0.windows.1
[Openvpn-devel] [PATCH applied] Re: Refactor OpenVPNService state detection code
(Basically just reworking if/goto into if/else, plus more verbose comments. "Git show -w --color-moved=zebra" helps in seeing "nothing really changed")
Your patch has been applied to the master branch.
commit 197f8076e43534f3dfe50e3c638143c38f4bad7f
Author: Simon Rozman Date: Sun Feb 24 19:15:44 2019 +0100
Refactor OpenVPNService state detection code
Signed-off-by: Simon Rozman <[email protected]...>
Acked-by: Gert Doering <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg18233.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering