| Message ID | 20171105223714.7852-1-simon@rozman.si |
|---|---|
| State | Superseded |
| Delegated to: | Selva Nair |
| Headers | show |
| Series | [Openvpn-devel,v2] openvpnserv: Add support for multi-instances | expand |
Hi, On Sun, Nov 5, 2017 at 5:37 PM, Simon Rozman <simon@rozman.si> wrote: > > While openvpn.exe can run multiple concurrent processes, openvpnserv.exe > is usually only one single globally unique running process. > > This patch extends openvpnserv.exe to support multiple service instances > in parallel allowing side-by-side OpenVPN installations. > > Alternate instances must be installed as `SERVICE_WIN32_OWN_PROCESS` > (Type 0x10) and must use the newly introduced service command line > parameter: > -instance <name> <id> > <name> can be `automatic` or `interactive`. Looks good and works as promised. Some comments + nitpicking below. > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c > index 4123d0f..ff5c1a2 100644 > --- a/src/openvpnserv/automatic.c > +++ b/src/openvpnserv/automatic.c > @@ -44,7 +44,7 @@ > #define false 0 > > static SERVICE_STATUS_HANDLE service; > -static SERVICE_STATUS status; > +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS }; While this is correct, making use of C99's designated init like {.dwServiceType = SERVICE_WIN32_SHARE_PROCESS} would be better and clearer. <snip> > if (!exit_event) > { > MsgToEventLog(M_ERR, TEXT("CreateEvent failed")); > @@ -327,8 +327,8 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv) > TEXT("%s\\%s"), settings.log_dir, log_file); > > /* construct command line */ > - openvpn_sntprintf(command_line, _countof(command_line), TEXT(PACKAGE " --service %s 1 --config \"%s\""), > - EXIT_EVENT_NAME, > + openvpn_sntprintf(command_line, _countof(command_line), TEXT("openvpn --service \"%s_exit_1\" 1 --config \"%s\""), > + service_instance, The change PACKAGE -> openvpn is not a regression because the first word of command line (argv[0]) is not used while launching the process, right? Instead, the exe_path is used to find the executable. However, argv[0] will no longer track PACKAGE but stay fixed at "openvpn". Hopefully no one cares. I'm fine with this as official builds see no difference. <snip> > diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c > index 0d162e8..649a9a8 100644 > --- a/src/openvpnserv/interactive.c > +++ b/src/openvpnserv/interactive.c > @@ -53,7 +53,7 @@ > #define ERROR_MESSAGE_TYPE 0x20000003 > > static SERVICE_STATUS_HANDLE service; > -static SERVICE_STATUS status; > +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS }; same as above... {.dwServiceType = ... } <snip> > diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c > index b79e999..232b33e 100644 > --- a/src/openvpnserv/service.c > +++ b/src/openvpnserv/service.c > @@ -223,46 +223,77 @@ out: > int > _tmain(int argc, TCHAR *argv[]) > { > - SERVICE_TABLE_ENTRY dispatchTable[] = { > + SERVICE_TABLE_ENTRY dispatchTable_shared[] = { > { automatic_service.name, ServiceStartAutomatic }, > { interactive_service.name, ServiceStartInteractive }, > { NULL, NULL } > }; > + const SERVICE_TABLE_ENTRY *dispatchTable = dispatchTable_shared; > > openvpn_service[0] = automatic_service; > openvpn_service[1] = interactive_service; > > - if (argc > 1 && (*argv[1] == TEXT('-') || *argv[1] == TEXT('/'))) > + for (int i = 1; i < argc; i++) > { > - if (_tcsicmp(TEXT("install"), argv[1] + 1) == 0) > + if (*argv[i] == TEXT('-') || *argv[i] == TEXT('/')) > { > - return CmdInstallServices(); > - } > - else if (_tcsicmp(TEXT("remove"), argv[1] + 1) == 0) > - { > - return CmdRemoveServices(); > - } > - else if (_tcsicmp(TEXT("start"), argv[1] + 1) == 0) > - { > - BOOL is_auto = argc < 3 || _tcsicmp(TEXT("interactive"), argv[2]) != 0; > - return CmdStartService(is_auto ? automatic : interactive); > - } > - else > - { > - goto dispatch; > + if (_tcsicmp(TEXT("install"), argv[i] + 1) == 0) > + { > + return CmdInstallServices(); > + } > + else if (_tcsicmp(TEXT("remove"), argv[i] + 1) == 0) > + { > + return CmdRemoveServices(); > + } > + else if (_tcsicmp(TEXT("start"), argv[i] + 1) == 0) > + { > + BOOL is_auto = argc < i + 2 || _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0; > + return CmdStartService(is_auto ? automatic : interactive); > + } > + else if (argc > i + 2 && _tcsicmp(TEXT("instance"), argv[i] + 1) == 0) > + { > + /* When running as an alternate instance, we are invoked as > + * SERVICE_WIN32_OWN_PROCESS - we are the one and only > + * service in this process. The dispatch table should > + * reflect that. > + */ > + static const SERVICE_TABLE_ENTRY dispatchTable_automatic[] = { > + { TEXT(""), ServiceStartAutomaticOwn }, > + { NULL, NULL } > + }; Agreed this array has to live beyond the for loop, but why static? Statics live for ever while this need not exist beyond the function. Putting it at the top where dispatchTable_shared is defined (or anywhere before the loop) as a non-static variable would be the way to go? > + static const SERVICE_TABLE_ENTRY dispatchTable_interactive[] = { > + { TEXT(""), ServiceStartInteractiveOwn }, > + { NULL, NULL } > + }; Same here. <snip> Finally, we could add the instance name to the eventlog output. MsgToEventLog prints errors as "APPNAME error: ...." where APPNAME is "openvpnserv". E.g., add " (instance_name)" after APPNAME? Thanks, Selva <div dir="ltr">Hi,<br><br>On Sun, Nov 5, 2017 at 5:37 PM, Simon Rozman <<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>> wrote:<br>><br>> While openvpn.exe can run multiple concurrent processes, openvpnserv.exe<br>> is usually only one single globally unique running process.<br>><br>> This patch extends openvpnserv.exe to support multiple service instances<br>> in parallel allowing side-by-side OpenVPN installations.<br>><br>> Alternate instances must be installed as `SERVICE_WIN32_OWN_PROCESS`<br>> (Type 0x10) and must use the newly introduced service command line<br>> parameter:<br>> -instance <name> <id><br>> <name> can be `automatic` or `interactive`.<div><br></div><div>Looks good and works as promised.<br></div><div><br></div><div>Some comments + nitpicking below.</div><div><br>> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c<br>> index 4123d0f..ff5c1a2 100644<br>> --- a/src/openvpnserv/automatic.c<br>> +++ b/src/openvpnserv/automatic.c<br>> @@ -44,7 +44,7 @@<br>> #define false 0<br>><br>> static SERVICE_STATUS_HANDLE service;<br>> -static SERVICE_STATUS status;<br>> +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS };<br><br>While this is correct, making use of C99's designated init like</div><div><br> {.dwServiceType = SERVICE_WIN32_SHARE_PROCESS}<br><br></div><div>would be better and clearer.<br><br></div><div><snip></div><div><br>> if (!exit_event)<br>> {<br>> MsgToEventLog(M_ERR, TEXT("CreateEvent failed"));<br>> @@ -327,8 +327,8 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)<br>> TEXT("%s\\%s"), settings.log_dir, log_file);<br>><br>> /* construct command line */<br>> - openvpn_sntprintf(command_lin<wbr>e, _countof(command_line), TEXT(PACKAGE " --service %s 1 --config \"%s\""),<br>> - EXIT_EVENT_NAME,<br>> + openvpn_sntprintf(command_lin<wbr>e, _countof(command_line), TEXT("openvpn --service \"%s_exit_1\" 1 --config \"%s\""),<br>> + service_instance,<br><br><div>The change PACKAGE -> openvpn is not a regression because the</div><div>first word of command line (argv[0]) is not used while launching the process,</div><div>right? Instead, the exe_path is used to find the executable. </div><div><br></div><div>However, argv[0] will no longer track PACKAGE but stay fixed at "openvpn".</div><div>Hopefully no one cares. I'm fine with this as official builds see no difference.</div><div><br><snip></div><div><br>> diff --git a/src/openvpnserv/interactive.<wbr>c b/src/openvpnserv/interactive.<wbr>c<br>> index 0d162e8..649a9a8 100644<br>> --- a/src/openvpnserv/interactive.<wbr>c<br>> +++ b/src/openvpnserv/interactive.<wbr>c<br>> @@ -53,7 +53,7 @@<br>> #define ERROR_MESSAGE_TYPE 0x20000003<br>><br>> static SERVICE_STATUS_HANDLE service;<br>> -static SERVICE_STATUS status;<br>> +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS };<br><br>same as above... {.dwServiceType = ... }<br><br></div><div><snip></div><div> <br>> diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c<br>> index b79e999..232b33e 100644<br>> --- a/src/openvpnserv/service.c<br>> +++ b/src/openvpnserv/service.c<br>> @@ -223,46 +223,77 @@ out:<br>> int<br>> _tmain(int argc, TCHAR *argv[])<br>> {<br>> - SERVICE_TABLE_ENTRY dispatchTable[] = {<br>> + SERVICE_TABLE_ENTRY dispatchTable_shared[] = {<br>> { <a href="http://automatic_service.name" target="_blank">automatic_service.name</a>, ServiceStartAutomatic },<br>> { <a href="http://interactive_service.name" target="_blank">interactive_service.name</a>, ServiceStartInteractive },<br>> { NULL, NULL }<br>> };<br>> + const SERVICE_TABLE_ENTRY *dispatchTable = dispatchTable_shared;<br>><br>> openvpn_service[0] = automatic_service;<br>> openvpn_service[1] = interactive_service;<br>><br>> - if (argc > 1 && (*argv[1] == TEXT('-') || *argv[1] == TEXT('/')))<br>> + for (int i = 1; i < argc; i++)<br>> {<br>> - if (_tcsicmp(TEXT("install"), argv[1] + 1) == 0)<br>> + if (*argv[i] == TEXT('-') || *argv[i] == TEXT('/'))<br>> {<br>> - return CmdInstallServices();<br>> - }<br>> - else if (_tcsicmp(TEXT("remove"), argv[1] + 1) == 0)<br>> - {<br>> - return CmdRemoveServices();<br>> - }<br>> - else if (_tcsicmp(TEXT("start"), argv[1] + 1) == 0)<br>> - {<br>> - BOOL is_auto = argc < 3 || _tcsicmp(TEXT("interactive"), argv[2]) != 0;<br>> - return CmdStartService(is_auto ? automatic : interactive);<br>> - }<br>> - else<br>> - {<br>> - goto dispatch;<br>> + if (_tcsicmp(TEXT("install"), argv[i] + 1) == 0)<br>> + {<br>> + return CmdInstallServices();<br>> + }<br>> + else if (_tcsicmp(TEXT("remove"), argv[i] + 1) == 0)<br>> + {<br>> + return CmdRemoveServices();<br>> + }<br>> + else if (_tcsicmp(TEXT("start"), argv[i] + 1) == 0)<br>> + {<br>> + BOOL is_auto = argc < i + 2 || _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0;<br>> + return CmdStartService(is_auto ? automatic : interactive);<br>> + }<br>> + else if (argc > i + 2 && _tcsicmp(TEXT("instance"), argv[i] + 1) == 0)<br>> + {<br>> + /* When running as an alternate instance, we are invoked as<br>> + * SERVICE_WIN32_OWN_PROCESS - we are the one and only<br>> + * service in this process. The dispatch table should<br>> + * reflect that.<br>> + */<br>> + static const SERVICE_TABLE_ENTRY dispatchTable_automatic[] = {<br>> + { TEXT(""), ServiceStartAutomaticOwn },<br>> + { NULL, NULL }<br>> + };<div><br></div><div>Agreed this array has to live beyond the for loop, but why static? Statics</div><div>live for ever while this need not exist beyond the function. Putting it at the</div><div>top where dispatchTable_shared is defined (or anywhere before the loop)</div><div>as a non-static variable would be the way to go?</div><div><br>> + static const SERVICE_TABLE_ENTRY dispatchTable_interactive[] = {<br>> + { TEXT(""), ServiceStartInteractiveOwn },<br>> + { NULL, NULL }<br>> + };</div><div><br></div><div>Same here.</div><div><br></div><div><snip></div><div><br></div><div>Finally, we could add the instance name to the eventlog output. MsgToEventLog</div><div>prints errors as "APPNAME error: ...." where APPNAME is "openvpnserv".</div><div>E.g., add " (instance_name)" after APPNAME?</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva<br></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > > static SERVICE_STATUS_HANDLE service; -static SERVICE_STATUS status; > > +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS }; > > While this is correct, making use of C99's designated init like > > {.dwServiceType = SERVICE_WIN32_SHARE_PROCESS} would be better > and clearer. OK. > > /* construct command line */ > > - openvpn_sntprintf(command_line, > > _countof(command_line), TEXT(PACKAGE " --service %s 1 --config > > \"%s\""), > > - EXIT_EVENT_NAME, > > + openvpn_sntprintf(command_line, > > + _countof(command_line), TEXT("openvpn --service \"%s_exit_1\" 1 > > + --config \"%s\""), > > + service_instance, > The change PACKAGE -> openvpn is not a regression because the first word > of command line (argv[0]) is not used while launching the process, right? > Instead, the exe_path is used to find the executable. > > However, argv[0] will no longer track PACKAGE but stay fixed at "openvpn". > Hopefully no one cares. I'm fine with this as official builds see no difference. When executable path is specified explicitly at CreateProcess()/CreateProcessAsUser(), the argv[0] of the command line is not used. It does get passed to the newly created openvpn.exe process as the argv[0], although the openvpn.exe ignores argv[0]. So the argv[0] can be "openvpn", "openvpn.exe", PACKAGE, "foobar", whatever... The interactive service is using hardcoded "openvpn". While changing the command line for the automatic service to use dynamic instance name for exit event, I synchronized argv[0] to "openvpn" as well. Indeed, for official builds that does not change anything. > > + static const SERVICE_TABLE_ENTRY > > + dispatchTable_automatic[] = { > > + { TEXT(""), ServiceStartAutomaticOwn }, > > + { NULL, NULL } > > + }; > > Agreed this array has to live beyond the for loop, but why static? Statics live > for ever while this need not exist beyond the function. Putting it at the top > where dispatchTable_shared is defined (or anywhere before the loop) as a > non-static variable would be the way to go? - There is almost nothing beyond this function. This is "the" main function. This function endures the whole process lifetime, making its local variables persist on stack the whole process lifetime. Not much better than static variables. - If this was some big chunk of data declared as a main() local variable, it would eat up its chunk of stack for the whole process lifetime. When it is declared as static, it is kept in the data segment. - But the main reason behind static usage is to allow us to keep the feature as a single chunk of code vs. splitting it across the main function. I personally put code aesthetics before memory usage. > Finally, we could add the instance name to the eventlog output. > MsgToEventLog prints errors as "APPNAME error: ...." where APPNAME is > "openvpnserv". > E.g., add " (instance_name)" after APPNAME? Excellent suggestion. I shall add it to the v3 patch. However, I propose a bit different output. The `service_instance` dynamic service name is used as a replacement for "OpenVPN" and "openvpn" hardcoded strings in the code. Therefore, logging "APPNAME (instance_name)" would log as "OpenVPN (OpenVPN)" for official builds. Not exactly elegant. I would prefer to keep the official OpenVPN log output backward binary compatible. An example of how we branded interactive service instance for eduVPN: - The interactive service is installed as "OpenVPNServiceInteractive$eduVPN" with the display name "OpenVPN Interactive Service (eduVPN)" - The service is launched using "openvpnserv.exe -instance interactive OpenVPN$eduVPN" command line. - This makes it load the settings from "HKEY_LOCAL_MACHINE\SOFTWARE\OpenVPN$eduVPN" We were careful to start the instance name with "OpenVPN$" because: - We don't want to hide the OpenVPN interactive service official name/brand. - Our version of interactive service is always listed next to the official OpenVPN's. In the registry and on the services.msc list. - Our registry key is adjacent to the official OpenVPN's. - $ is also used in Microsoft SQL Server as a delimiter. Using the APPNAME ("openvpnserv") and merging with our instance name would make a Frankenstein "openvpnservOpenVPN$eduVPN" log entry. Not exactly elegant either. For v3 I shall redesign the code to use compile-time defined PACKAGE_NAME/PACKAGE as the left part again, and only append the "" service_instance for official OpenVPN and "$eduVPN" for named instance. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi > > > > + static const SERVICE_TABLE_ENTRY > > > + dispatchTable_automatic[] = { > > > + { TEXT(""), ServiceStartAutomaticOwn }, > > > + { NULL, NULL } > > > + }; > > > > Agreed this array has to live beyond the for loop, but why static? > Statics live > > for ever while this need not exist beyond the function. Putting it at > the top > > where dispatchTable_shared is defined (or anywhere before the loop) as a > > non-static variable would be the way to go? > > - There is almost nothing beyond this function. This is "the" main > function. This function endures the whole process lifetime, making its > local variables persist on stack the whole process lifetime. Not much > better than static variables. > > - If this was some big chunk of data declared as a main() local variable, > it would eat up its chunk of stack for the whole process lifetime. When it > is declared as static, it is kept in the data segment. > > - But the main reason behind static usage is to allow us to keep the > feature as a single chunk of code vs. splitting it across the main > function. I personally put code aesthetics before memory usage. > I guessed that much. And agreed that this is main and everything here persists for ever... Also minimizing the scope of declarations is good style. But then making the variable static just to keep a valid pointer beyond the current block local looks like a kludge. For me seeing static applied to a variable scoped to a block is just confusing and unusual style. Think of this: if you remove that static the code may still build and even work on some compilers depending on optimization level etc. and mysteriously fail on some occasions. From that one could either conclude a static qualifier is required her or the variable is wrongly scoped. I think the latter conclusion is the 'right' one and static is a misuse. As for the combination 'static const ...', it has a place and that is for constants defined outside functions to limit the scope of an otherwise global const to that of the compilation unit. It may be just me. > > > Finally, we could add the instance name to the eventlog output. > > MsgToEventLog prints errors as "APPNAME error: ...." where APPNAME is > > "openvpnserv". > > E.g., add " (instance_name)" after APPNAME? > > Excellent suggestion. I shall add it to the v3 patch. > > However, I propose a bit different output. The `service_instance` dynamic > service name is used as a replacement for "OpenVPN" and "openvpn" hardcoded > strings in the code. Therefore, logging "APPNAME (instance_name)" would log > as "OpenVPN (OpenVPN)" for official builds. Not exactly elegant. I would > prefer to keep the official OpenVPN log output backward binary compatible. > > An example of how we branded interactive service instance for eduVPN: > - The interactive service is installed as "OpenVPNServiceInteractive$eduVPN" > with the display name "OpenVPN Interactive Service (eduVPN)" > Use of $instance is MSSQL style... good. > - The service is launched using "openvpnserv.exe -instance interactive > OpenVPN$eduVPN" command line. > - This makes it load the settings from "HKEY_LOCAL_MACHINE\SOFTWARE\ > OpenVPN$eduVPN" > > We were careful to start the instance name with "OpenVPN$" because: > - We don't want to hide the OpenVPN interactive service official > name/brand. > - Our version of interactive service is always listed next to the official > OpenVPN's. In the registry and on the services.msc list. > - Our registry key is adjacent to the official OpenVPN's. > - $ is also used in Microsoft SQL Server as a delimiter. > I like it: using "OpenVPN" as a kind of namespace also avoids name collision with any other 'foobar' installed in HKLM\Software\foobar > > Using the APPNAME ("openvpnserv") and merging with our instance name would > make a Frankenstein "openvpnservOpenVPN$eduVPN" log entry. Not exactly > elegant either. > > For v3 I shall redesign the code to use compile-time defined > PACKAGE_NAME/PACKAGE as the left part again, and only append the "" > service_instance for official OpenVPN and "$eduVPN" for named instance. > Not sure I follow, but will wait for the patch. I suppose naming additional instances as, say, 'foo' using the official package and have service pipe, exit event etc named after it and registry keys taken from HKLM\Software\foo will still work. In fact I like the current patch in that it allows installing even the official instance as a named one with name = OpenVPN and thus decouples automatic and interactive services. That makes it easy to avoid installing the legacy service, which I think no one uses anymore. Thanks, Selva <div dir="ltr">Hi<div><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> <br> > > + static const SERVICE_TABLE_ENTRY<br> > > + dispatchTable_automatic[] = {<br> <span class="">> > + { TEXT(""), ServiceStartAutomaticOwn },<br> > > + { NULL, NULL }<br> > > + };<br> ><br> > Agreed this array has to live beyond the for loop, but why static? Statics live<br> > for ever while this need not exist beyond the function. Putting it at the top<br> > where dispatchTable_shared is defined (or anywhere before the loop) as a<br> > non-static variable would be the way to go?<br> <br> </span>- There is almost nothing beyond this function. This is "the" main function. This function endures the whole process lifetime, making its local variables persist on stack the whole process lifetime. Not much better than static variables.<br> <br> - If this was some big chunk of data declared as a main() local variable, it would eat up its chunk of stack for the whole process lifetime. When it is declared as static, it is kept in the data segment.<br> <br> - But the main reason behind static usage is to allow us to keep the feature as a single chunk of code vs. splitting it across the main function. I personally put code aesthetics before memory usage.<br></blockquote><div><br></div><div>I guessed that much. And agreed that this is main and everything here persists</div><div>for ever... Also minimizing the scope of declarations is good style.</div><div><br></div><div>But then making the variable static just to keep a valid pointer beyond the current block</div><div>local looks like a kludge. For me seeing static applied to a variable scoped to</div><div>a block is just confusing and unusual style. Think of this: if you remove that static</div><div>the code may still build and even work on some compilers depending on optimization</div><div>level etc. and mysteriously fail on some occasions. From that one could either conclude</div><div>a static qualifier is required her or the variable is wrongly scoped. I think the latter</div><div>conclusion is the 'right' one and static is a misuse.</div><div><br></div><div>As for the combination 'static const ...', it has a place and that is for constants</div><div>defined outside functions to limit the scope of an otherwise global const to</div><div>that of the compilation unit.</div><div><br></div><div>It may be just me.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <span class=""><br> <br> > Finally, we could add the instance name to the eventlog output.<br> > MsgToEventLog prints errors as "APPNAME error: ...." where APPNAME is<br> > "openvpnserv".<br> > E.g., add " (instance_name)" after APPNAME?<br> <br> </span>Excellent suggestion. I shall add it to the v3 patch.<br> <br> However, I propose a bit different output. The `service_instance` dynamic service name is used as a replacement for "OpenVPN" and "openvpn" hardcoded strings in the code. Therefore, logging "APPNAME (instance_name)" would log as "OpenVPN (OpenVPN)" for official builds. Not exactly elegant. I would prefer to keep the official OpenVPN log output backward binary compatible.<br> <br> An example of how we branded interactive service instance for eduVPN:<br> - The interactive service is installed as "OpenVPNServiceInteractive$<wbr>eduVPN" with the display name "OpenVPN Interactive Service (eduVPN)"<br></blockquote><div><br></div><div>Use of $instance is MSSQL style... good.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - The service is launched using "openvpnserv.exe -instance interactive OpenVPN$eduVPN" command line.<br> - This makes it load the settings from "HKEY_LOCAL_MACHINE\SOFTWARE\<wbr>OpenVPN$eduVPN"<br> <br> We were careful to start the instance name with "OpenVPN$" because:<br> - We don't want to hide the OpenVPN interactive service official name/brand.<br> - Our version of interactive service is always listed next to the official OpenVPN's. In the registry and on the services.msc list.<br> - Our registry key is adjacent to the official OpenVPN's.<br> - $ is also used in Microsoft SQL Server as a delimiter.<br></blockquote><div><br></div><div>I like it: using "OpenVPN" as a kind of namespace also avoids name collision with</div><div>any other 'foobar' installed in HKLM\Software\foobar</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Using the APPNAME ("openvpnserv") and merging with our instance name would make a Frankenstein "openvpnservOpenVPN$eduVPN" log entry. Not exactly elegant either.<br> <br> For v3 I shall redesign the code to use compile-time defined PACKAGE_NAME/PACKAGE as the left part again, and only append the "" service_instance for official OpenVPN and "$eduVPN" for named instance.<br></blockquote><div><br></div><div>Not sure I follow, but will wait for the patch. I suppose naming additional</div><div>instances as, say, 'foo' using the official package and have service pipe, exit</div><div>event etc named after it and registry keys taken from HKLM\Software\foo will</div><div>still work.</div><div><br></div><div>In fact I like the current patch in that it allows installing even the official instance</div><div>as a named one with name = OpenVPN and thus decouples automatic and</div><div>interactive services. That makes it easy to avoid installing the legacy service,</div><div>which I think no one uses anymore.</div><div> </div><div>Thanks,</div><div><br></div><div>Selva</div></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, > But then making the variable static just to keep a valid pointer beyond the > current block local looks like a kludge. For me seeing static applied to a > variable scoped to a block is just confusing and unusual style. Think of this: if > you remove that static the code may still build and even work on some > compilers depending on optimization level etc. and mysteriously fail on some > occasions. From that one could either conclude a static qualifier is required > her or the variable is wrongly scoped. I think the latter conclusion is the 'right' > one and static is a misuse. > > As for the combination 'static const ...', it has a place and that is for constants > defined outside functions to limit the scope of an otherwise global const to > that of the compilation unit. > > It may be just me. Not necessarily. It may be just *me*. :) Anyway, you got me convinced and I shall move those structs from data segment to stack in the next version of the patch. > I like it: using "OpenVPN" as a kind of namespace also avoids name collision > with any other 'foobar' installed in HKLM\Software\foobar Exactly, introducing instances is all about avoiding namespace collisions. > Not sure I follow, but will wait for the patch. I suppose naming additional > instances as, say, 'foo' using the official package and have service pipe, exit > event etc named after it and registry keys taken from HKLM\Software\foo > will still work. In the v1 and v2 patches, yes. But in v3 patch, the PACKAGE/PACKAGE_NAME is always used as the prefix/namespace. This way we impose the "OpenVPN" branding to all instances by compile-time default. In v3, people can't take the interactive service and make it disguise as the "Microsoft\Office\Automatic Update". The v3 made instancing less flexible on purpose. If you disagree, I can revert to v1/2 behaviour. > In fact I like the current patch in that it allows installing even the official > instance as a named one with name = OpenVPN and thus decouples > automatic and interactive services. That makes it easy to avoid installing the > legacy service, which I think no one uses anymore. Ouch. *We* would use the automatic service if it was coded slightly different. I didn't know the legacy automatic service is on the obsolete list yet. Perhaps, a true replacement for the automatic service could be to make the openvpn.exe itself to be able to install and run as a Windows service (--daemon for Windows): - The SCM would restart it, should openvpn.exe terminate or crash. - Separate openvpn.exe service for each ovpn configuration, allowing admins to manage them individually. - Linux like behaviour, where you run each tunnel as a separate Systemd daemon. (That is exactly what I'd need for our office. Right now we are wrapping openvpn.exe inside a NSSM service helper. Would be nice to get that feature out-of-the-box.) Back to the interactive service... According to the latest [PATCH( v3 missing)], one would need to start the default instance of the interactive service only using the following service command line: openvpnserv.exe -instance interactive "" A bit odd for the command line. Again, I can revert to v1/2 behaviour and let the end-users select the whole instance name at run-time. Best regards, Simon ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi Simon, On Thu, Nov 9, 2017 at 3:33 AM, Simon Rozman <simon@rozman.si> wrote: > Hi, > > > But then making the variable static just to keep a valid pointer beyond > the > > current block local looks like a kludge. For me seeing static applied to > a > > variable scoped to a block is just confusing and unusual style. Think of > this: if > > you remove that static the code may still build and even work on some > > compilers depending on optimization level etc. and mysteriously fail on > some > > occasions. From that one could either conclude a static qualifier is > required > > her or the variable is wrongly scoped. I think the latter conclusion is > the 'right' > > one and static is a misuse. > > > > As for the combination 'static const ...', it has a place and that is > for constants > > defined outside functions to limit the scope of an otherwise global > const to > > that of the compilation unit. > > > > It may be just me. > > Not necessarily. It may be just *me*. :) > > Anyway, you got me convinced and I shall move those structs from data > segment to stack in the next version of the patch. Glad that I don't have to invent an Acked-with-reservations: tag :) > > > I like it: using "OpenVPN" as a kind of namespace also avoids name > collision > > with any other 'foobar' installed in HKLM\Software\foobar > > Exactly, introducing instances is all about avoiding namespace collisions. > > > Not sure I follow, but will wait for the patch. I suppose naming > additional > > instances as, say, 'foo' using the official package and have service > pipe, exit > > event etc named after it and registry keys taken from HKLM\Software\foo > > will still work. > > In the v1 and v2 patches, yes. But in v3 patch, the PACKAGE/PACKAGE_NAME > is always used as the prefix/namespace. This way we impose the "OpenVPN" > branding to all instances by compile-time default. In v3, people can't take > the interactive service and make it disguise as the > "Microsoft\Office\Automatic Update". > > The v3 made instancing less flexible on purpose. > > If you disagree, I can revert to v1/2 behaviour. > Looked through v3 changes --- either approach (v2 or v3) is fine. > > > In fact I like the current patch in that it allows installing even the > official > > instance as a named one with name = OpenVPN and thus decouples > > automatic and interactive services. That makes it easy to avoid > installing the > > legacy service, which I think no one uses anymore. > > Ouch. *We* would use the automatic service if it was coded slightly > different. I didn't know the legacy automatic service is on the obsolete > list yet. > The automatic service per-se will continue to be there. But now its served by the C#/.NET version (https://github.com/OpenVPN/openvpnserv2) that is included in the distribution as openvpnserv2.exe We kept the old service renamed from openvpnservice to openvpnservicelegacy as a precaution. > Perhaps, a true replacement for the automatic service could be to make the > openvpn.exe itself to be able to install and run as a Windows service > (--daemon for Windows): > - The SCM would restart it, should openvpn.exe terminate or crash. > - Separate openvpn.exe service for each ovpn configuration, allowing > admins to manage them individually. > - Linux like behaviour, where you run each tunnel as a separate Systemd > daemon. > Essentially that's what the new automatic service does except for the ability to selectively control (start/stop) individual configs. I too would like to see that functionality but .NET puts me to sleep. > > (That is exactly what I'd need for our office. Right now we are wrapping > openvpn.exe inside a NSSM service helper. Would be nice to get that feature > out-of-the-box.) > Right now using nssm is somewhat more versatile than openvpnserv2 but the latter does watch the process, restarts if needed etc., which the original automatic service was not designed to do. At that time we had debated about nssm vs openvpnserv2.exe and the latter won because of some packaging convenience.. Personally I would like an automatic service that runs with limited privileges (e.g., as LocalService) which uses the interactive service to start instances, watches over the process and handles individual configs independently. Refactoring the code in automatic.c may be a way to achieve that. > Back to the interactive service... According to the latest [PATCH( v3 > missing)], one would need to start the default instance of the interactive > service only using the following service command line: > > openvpnserv.exe -instance interactive "" > Yes that would work in a pinch and, once installed, the user need not worry about the oddity of that command line. But ignore my comment about running the default instance as a named one. That's not really necessary. All said, v4 could be based on v3 or v2. Now try to convince Gert that this belongs to 2.4 :) Regards, Selva <div dir="ltr">Hi Simon,<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 9, 2017 at 3:33 AM, Simon Rozman <span dir="ltr"><<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <span class="gmail-"><br> > But then making the variable static just to keep a valid pointer beyond the<br> > current block local looks like a kludge. For me seeing static applied to a<br> > variable scoped to a block is just confusing and unusual style. Think of this: if<br> > you remove that static the code may still build and even work on some<br> > compilers depending on optimization level etc. and mysteriously fail on some<br> > occasions. From that one could either conclude a static qualifier is required<br> > her or the variable is wrongly scoped. I think the latter conclusion is the 'right'<br> > one and static is a misuse.<br> ><br> > As for the combination 'static const ...', it has a place and that is for constants<br> > defined outside functions to limit the scope of an otherwise global const to<br> > that of the compilation unit.<br> ><br> > It may be just me.<br> <br> </span>Not necessarily. It may be just *me*. :)<br> <br> Anyway, you got me convinced and I shall move those structs from data segment to stack in the next version of the patch.</blockquote><div><br></div><div> Glad that I don't have to invent an Acked-with-reservations: tag :) </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span class="gmail-"><br> > I like it: using "OpenVPN" as a kind of namespace also avoids name collision<br> > with any other 'foobar' installed in HKLM\Software\foobar<br> <br> </span>Exactly, introducing instances is all about avoiding namespace collisions.<br> <span class="gmail-"><br> > Not sure I follow, but will wait for the patch. I suppose naming additional<br> > instances as, say, 'foo' using the official package and have service pipe, exit<br> > event etc named after it and registry keys taken from HKLM\Software\foo<br> > will still work.<br> <br> </span>In the v1 and v2 patches, yes. But in v3 patch, the PACKAGE/PACKAGE_NAME is always used as the prefix/namespace. This way we impose the "OpenVPN" branding to all instances by compile-time default. In v3, people can't take the interactive service and make it disguise as the "Microsoft\Office\Automatic Update".<br> <br> The v3 made instancing less flexible on purpose.<br> <br> If you disagree, I can revert to v1/2 behaviour.<br></blockquote><div><br></div><div>Looked through v3 changes --- either approach (v2 or v3) is fine.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span class="gmail-"><br> > In fact I like the current patch in that it allows installing even the official<br> > instance as a named one with name = OpenVPN and thus decouples<br> > automatic and interactive services. That makes it easy to avoid installing the<br> > legacy service, which I think no one uses anymore.<br> <br> </span>Ouch. *We* would use the automatic service if it was coded slightly different. I didn't know the legacy automatic service is on the obsolete list yet.<br></blockquote><div><br></div><div>The automatic service per-se will continue to be there. But now its served</div><div>by the C#/.NET version (<a href="https://github.com/OpenVPN/openvpnserv2">https://github.com/OpenVPN/openvpnserv2</a>) that</div><div>is included in the distribution as openvpnserv2.exe We kept the old service</div><div>renamed from openvpnservice to openvpnservicelegacy as a precaution.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Perhaps, a true replacement for the automatic service could be to make the openvpn.exe itself to be able to install and run as a Windows service (--daemon for Windows):<br> - The SCM would restart it, should openvpn.exe terminate or crash.<br> - Separate openvpn.exe service for each ovpn configuration, allowing admins to manage them individually.<br> - Linux like behaviour, where you run each tunnel as a separate Systemd daemon.<br></blockquote><div><br></div><div>Essentially that's what the new automatic service does except for the ability to</div><div>selectively control (start/stop) individual configs. I too would like to see that</div><div>functionality but .NET puts me to sleep.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> (That is exactly what I'd need for our office. Right now we are wrapping openvpn.exe inside a NSSM service helper. Would be nice to get that feature out-of-the-box.)<br></blockquote><div><br></div><div>Right now using nssm is somewhat more versatile than openvpnserv2 but the latter</div><div>does watch the process, restarts if needed etc., which the original automatic</div><div>service was not designed to do. At that time we had debated about nssm vs</div><div>openvpnserv2.exe and the latter won because of some packaging convenience..</div><div><br></div><div>Personally I would like an automatic service that runs with limited privileges </div><div>(e.g., as LocalService) which uses the interactive service to start instances,</div><div>watches over the process and handles individual configs independently.</div><div>Refactoring the code in automatic.c may be a way to achieve that.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Back to the interactive service... According to the latest [PATCH( v3 missing)], one would need to start the default instance of the interactive service only using the following service command line:<br> <br> openvpnserv.exe -instance interactive ""<br></blockquote><div><br></div><div>Yes that would work in a pinch and, once installed, the user need not worry</div><div>about the oddity of that command line. But ignore my comment about running</div><div>the default instance as a named one. That's not really necessary.</div><div><br></div><div>All said, v4 could be based on v3 or v2. Now try to convince Gert that this</div><div>belongs to 2.4 :)</div><div><br></div><div>Regards,</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi,
(haven't read the rest of the thread yet, but this one caught my eye)
On Thu, Nov 09, 2017 at 11:12:25AM -0500, Selva wrote:
> Now try to convince Gert that this belongs to 2.4 :)
Well... where would the incentive be to upgrade to 2.5, if we put all
the nice stuff into 2.4...?
Jokes aside, 2.4 will be with us for a while, and if a patch here
means "joining forces with EduVPN, more testing, more bugfixing, better
software" that is quite a strong argument...
Let's wait for you two to agree on the code first :-)
gert
Hi Simon, IIRC, this patch is waiting for a new version to take care of the static const as agreed below: On Thu, Nov 9, 2017 at 11:12 AM, Selva <selva.nair@gmail.com> wrote: > Hi Simon, > > On Thu, Nov 9, 2017 at 3:33 AM, Simon Rozman <simon@rozman.si> wrote: > >> Hi, >> >> > But then making the variable static just to keep a valid pointer beyond >> the >> > current block local looks like a kludge. For me seeing static applied >> to a >> > variable scoped to a block is just confusing and unusual style. Think >> of this: if >> > you remove that static the code may still build and even work on some >> > compilers depending on optimization level etc. and mysteriously fail on >> some >> > occasions. From that one could either conclude a static qualifier is >> required >> > her or the variable is wrongly scoped. I think the latter conclusion is >> the 'right' >> > one and static is a misuse. >> > >> > As for the combination 'static const ...', it has a place and that is >> for constants >> > defined outside functions to limit the scope of an otherwise global >> const to >> > that of the compilation unit. >> > >> > It may be just me. >> >> Not necessarily. It may be just *me*. :) >> >> Anyway, you got me convinced and I shall move those structs from data >> segment to stack in the next version of the patch. > > > Glad that I don't have to invent an Acked-with-reservations: tag :) > I suppose a v4 is coming. Thanks, Selva <div dir="ltr">Hi Simon,<div><br></div><div>IIRC, this patch is waiting for a new version to take care of the static const as</div><div>agreed below:</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 9, 2017 at 11:12 AM, Selva <span dir="ltr"><<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Simon,<br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Nov 9, 2017 at 3:33 AM, Simon Rozman <span dir="ltr"><<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <span class="m_-5862847669999624797gmail-"><br> > But then making the variable static just to keep a valid pointer beyond the<br> > current block local looks like a kludge. For me seeing static applied to a<br> > variable scoped to a block is just confusing and unusual style. Think of this: if<br> > you remove that static the code may still build and even work on some<br> > compilers depending on optimization level etc. and mysteriously fail on some<br> > occasions. From that one could either conclude a static qualifier is required<br> > her or the variable is wrongly scoped. I think the latter conclusion is the 'right'<br> > one and static is a misuse.<br> ><br> > As for the combination 'static const ...', it has a place and that is for constants<br> > defined outside functions to limit the scope of an otherwise global const to<br> > that of the compilation unit.<br> ><br> > It may be just me.<br> <br> </span>Not necessarily. It may be just *me*. :)<br> <br> Anyway, you got me convinced and I shall move those structs from data segment to stack in the next version of the patch.</blockquote><div><br></div></span><div> Glad that I don't have to invent an Acked-with-reservations: tag :) </div></div></div></div></blockquote><div><br></div><div>I suppose a v4 is coming.</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c index 4123d0f..ff5c1a2 100644 --- a/src/openvpnserv/automatic.c +++ b/src/openvpnserv/automatic.c @@ -44,7 +44,7 @@ #define false 0 static SERVICE_STATUS_HANDLE service; -static SERVICE_STATUS status; +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS }; openvpn_service_t automatic_service = { automatic, @@ -60,12 +60,6 @@ struct security_attributes SECURITY_DESCRIPTOR sd; }; -/* - * Which registry key in HKLM should - * we get config info from? - */ -#define REG_KEY "SOFTWARE\\" PACKAGE_NAME - static HANDLE exit_event = NULL; /* clear an object */ @@ -91,15 +85,6 @@ init_security_attributes_allow_all(struct security_attributes *obj) return true; } -/* - * This event is initially created in the non-signaled - * state. It will transition to the signaled state when - * we have received a terminate signal from the Service - * Control Manager which will cause an asynchronous call - * of ServiceStop below. - */ -#define EXIT_EVENT_NAME TEXT(PACKAGE "_exit_1") - HANDLE create_event(LPCTSTR name, bool allow_all, bool initial_state, bool manual_reset) { @@ -212,10 +197,19 @@ ServiceCtrlAutomatic(DWORD ctrl_code, DWORD event, LPVOID data, LPVOID ctx) VOID WINAPI +ServiceStartAutomaticOwn(DWORD dwArgc, LPTSTR *lpszArgv) +{ + status.dwServiceType = SERVICE_WIN32_OWN_PROCESS; + ServiceStartAutomatic(dwArgc, lpszArgv); +} + + +VOID WINAPI ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv) { DWORD error = NO_ERROR; settings_t settings; + TCHAR event_name[256]; service = RegisterServiceCtrlHandlerEx(automatic_service.name, ServiceCtrlAutomatic, &status); if (!service) @@ -223,7 +217,6 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv) return; } - status.dwServiceType = SERVICE_WIN32_SHARE_PROCESS; status.dwCurrentState = SERVICE_START_PENDING; status.dwServiceSpecificExitCode = NO_ERROR; status.dwWin32ExitCode = NO_ERROR; @@ -237,8 +230,15 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv) /* * Create our exit event + * This event is initially created in the non-signaled + * state. It will transition to the signaled state when + * we have received a terminate signal from the Service + * Control Manager which will cause an asynchronous call + * of ServiceStop below. */ - exit_event = create_event(EXIT_EVENT_NAME, false, false, true); + + openvpn_sntprintf(event_name, _countof(event_name), TEXT("%s_exit_1"), service_instance); + exit_event = create_event(event_name, false, false, true); if (!exit_event) { MsgToEventLog(M_ERR, TEXT("CreateEvent failed")); @@ -327,8 +327,8 @@ ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv) TEXT("%s\\%s"), settings.log_dir, log_file); /* construct command line */ - openvpn_sntprintf(command_line, _countof(command_line), TEXT(PACKAGE " --service %s 1 --config \"%s\""), - EXIT_EVENT_NAME, + openvpn_sntprintf(command_line, _countof(command_line), TEXT("openvpn --service \"%s_exit_1\" 1 --config \"%s\""), + service_instance, find_obj.cFileName); /* Make security attributes struct for logfile handle so it can diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c index e77d7ab..dd5d058 100644 --- a/src/openvpnserv/common.c +++ b/src/openvpnserv/common.c @@ -24,6 +24,9 @@ #include "service.h" #include "validate.h" +LPCTSTR service_instance = TEXT(PACKAGE_NAME); + + /* * These are necessary due to certain buggy implementations of (v)snprintf, * that don't guarantee null termination for size > 0. @@ -53,8 +56,6 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...) return len; } -#define REG_KEY TEXT("SOFTWARE\\" PACKAGE_NAME) - static DWORD GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size) { @@ -69,7 +70,7 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size) if (status != ERROR_SUCCESS) { SetLastError(status); - return MsgToEventLog(M_SYSERR, TEXT("Error querying registry value: HKLM\\%s\\%s"), REG_KEY, value); + return MsgToEventLog(M_SYSERR, TEXT("Error querying registry value: HKLM\\SOFTWARE\\%s\\%s"), service_instance, value); } return ERROR_SUCCESS; @@ -79,16 +80,19 @@ GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size) DWORD GetOpenvpnSettings(settings_t *s) { + TCHAR reg_path[256]; TCHAR priority[64]; TCHAR append[2]; DWORD error; HKEY key; - LONG status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, REG_KEY, 0, KEY_READ, &key); + openvpn_sntprintf(reg_path, _countof(reg_path), TEXT("SOFTWARE\\%s"), service_instance); + + LONG status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, reg_path, 0, KEY_READ, &key); if (status != ERROR_SUCCESS) { SetLastError(status); - return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key HKLM\\%s not found"), REG_KEY); + return MsgToEventLog(M_SYSERR, TEXT("Could not open Registry key HKLM\\%s not found"), reg_path); } error = GetRegString(key, TEXT("exe_path"), s->exe_path, sizeof(s->exe_path)); diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 0d162e8..649a9a8 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -53,7 +53,7 @@ #define ERROR_MESSAGE_TYPE 0x20000003 static SERVICE_STATUS_HANDLE service; -static SERVICE_STATUS status; +static SERVICE_STATUS status = { SERVICE_WIN32_SHARE_PROCESS }; static HANDLE exit_event = NULL; static settings_t settings; static HANDLE rdns_semaphore = NULL; @@ -1487,7 +1487,7 @@ RunOpenvpn(LPVOID p) } openvpn_sntprintf(ovpn_pipe_name, _countof(ovpn_pipe_name), - TEXT("\\\\.\\pipe\\openvpn\\service_%lu"), GetCurrentThreadId()); + TEXT("\\\\.\\pipe\\%s\\service_%lu"), service_instance, GetCurrentThreadId()); ovpn_pipe = CreateNamedPipe(ovpn_pipe_name, PIPE_ACCESS_DUPLEX | FILE_FLAG_FIRST_PIPE_INSTANCE | FILE_FLAG_OVERLAPPED, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT, 1, 128, 128, 0, NULL); @@ -1654,6 +1654,7 @@ ServiceCtrlInteractive(DWORD ctrl_code, DWORD event, LPVOID data, LPVOID ctx) static HANDLE CreateClientPipeInstance(VOID) { + TCHAR pipe_name[256]; /* The entire pipe name string can be up to 256 characters long according to MSDN. */ HANDLE pipe = NULL; PACL old_dacl, new_dacl; PSECURITY_DESCRIPTOR sd; @@ -1690,7 +1691,8 @@ CreateClientPipeInstance(VOID) initialized = TRUE; } - pipe = CreateNamedPipe(TEXT("\\\\.\\pipe\\openvpn\\service"), flags, + openvpn_sntprintf(pipe_name, _countof(pipe_name), TEXT("\\\\.\\pipe\\%s\\service"), service_instance); + pipe = CreateNamedPipe(pipe_name, flags, PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE, PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, NULL); if (pipe == INVALID_HANDLE_VALUE) @@ -1785,6 +1787,15 @@ CmpHandle(LPVOID item, LPVOID hnd) return item == hnd; } + +VOID WINAPI +ServiceStartInteractiveOwn(DWORD dwArgc, LPTSTR *lpszArgv) +{ + status.dwServiceType = SERVICE_WIN32_OWN_PROCESS; + ServiceStartInteractive(dwArgc, lpszArgv); +} + + VOID WINAPI ServiceStartInteractive(DWORD dwArgc, LPTSTR *lpszArgv) { @@ -1801,7 +1812,6 @@ ServiceStartInteractive(DWORD dwArgc, LPTSTR *lpszArgv) return; } - status.dwServiceType = SERVICE_WIN32_SHARE_PROCESS; status.dwCurrentState = SERVICE_START_PENDING; status.dwServiceSpecificExitCode = NO_ERROR; status.dwWin32ExitCode = NO_ERROR; diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c index b79e999..232b33e 100644 --- a/src/openvpnserv/service.c +++ b/src/openvpnserv/service.c @@ -223,46 +223,77 @@ out: int _tmain(int argc, TCHAR *argv[]) { - SERVICE_TABLE_ENTRY dispatchTable[] = { + SERVICE_TABLE_ENTRY dispatchTable_shared[] = { { automatic_service.name, ServiceStartAutomatic }, { interactive_service.name, ServiceStartInteractive }, { NULL, NULL } }; + const SERVICE_TABLE_ENTRY *dispatchTable = dispatchTable_shared; openvpn_service[0] = automatic_service; openvpn_service[1] = interactive_service; - if (argc > 1 && (*argv[1] == TEXT('-') || *argv[1] == TEXT('/'))) + for (int i = 1; i < argc; i++) { - if (_tcsicmp(TEXT("install"), argv[1] + 1) == 0) + if (*argv[i] == TEXT('-') || *argv[i] == TEXT('/')) { - return CmdInstallServices(); - } - else if (_tcsicmp(TEXT("remove"), argv[1] + 1) == 0) - { - return CmdRemoveServices(); - } - else if (_tcsicmp(TEXT("start"), argv[1] + 1) == 0) - { - BOOL is_auto = argc < 3 || _tcsicmp(TEXT("interactive"), argv[2]) != 0; - return CmdStartService(is_auto ? automatic : interactive); - } - else - { - goto dispatch; + if (_tcsicmp(TEXT("install"), argv[i] + 1) == 0) + { + return CmdInstallServices(); + } + else if (_tcsicmp(TEXT("remove"), argv[i] + 1) == 0) + { + return CmdRemoveServices(); + } + else if (_tcsicmp(TEXT("start"), argv[i] + 1) == 0) + { + BOOL is_auto = argc < i + 2 || _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0; + return CmdStartService(is_auto ? automatic : interactive); + } + else if (argc > i + 2 && _tcsicmp(TEXT("instance"), argv[i] + 1) == 0) + { + /* When running as an alternate instance, we are invoked as + * SERVICE_WIN32_OWN_PROCESS - we are the one and only + * service in this process. The dispatch table should + * reflect that. + */ + static const SERVICE_TABLE_ENTRY dispatchTable_automatic[] = { + { TEXT(""), ServiceStartAutomaticOwn }, + { NULL, NULL } + }; + static const SERVICE_TABLE_ENTRY dispatchTable_interactive[] = { + { TEXT(""), ServiceStartInteractiveOwn }, + { NULL, NULL } + }; + dispatchTable = _tcsicmp(TEXT("interactive"), argv[i + 1]) != 0 ? + dispatchTable_automatic : + dispatchTable_interactive; + + service_instance = argv[i + 2]; + i += 2; + } + else + { + _tprintf(TEXT("%s -install to install the services\n"), APPNAME); + _tprintf(TEXT("%s -start <name> to start a service (\"automatic\" or \"interactive\")\n"), APPNAME); + _tprintf(TEXT("%s -remove to remove the services\n"), APPNAME); + + _tprintf(TEXT("\nService run-time parameters:\n")); + _tprintf(TEXT("-instance <name> <id>\n") + TEXT(" Runs the service as an alternate instance. <name> can be \"automatic\" or\n") + TEXT(" \"interactive\". The service settings will be loaded from\n") + TEXT(" HKLM\\Software\\<id> registry key, and the interactive service will accept\n") + TEXT(" requests on \\\\.\\pipe\\<id>\\service named pipe.\n")); + + return 0; + } } - - return 0; } /* If it doesn't match any of the above parameters * the service control manager may be starting the service * so we must call StartServiceCtrlDispatcher */ -dispatch: - _tprintf(TEXT("%s -install to install the services\n"), APPNAME); - _tprintf(TEXT("%s -start <name> to start a service (\"automatic\" or \"interactive\")\n"), APPNAME); - _tprintf(TEXT("%s -remove to remove the services\n"), APPNAME); _tprintf(TEXT("\nStartServiceCtrlDispatcher being called.\n")); _tprintf(TEXT("This may take several seconds. Please wait.\n")); diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h index 9fe573e..1afd20e 100644 --- a/src/openvpnserv/service.h +++ b/src/openvpnserv/service.h @@ -73,10 +73,12 @@ typedef struct { extern openvpn_service_t automatic_service; extern openvpn_service_t interactive_service; +extern LPCTSTR service_instance; - +VOID WINAPI ServiceStartAutomaticOwn(DWORD argc, LPTSTR *argv); VOID WINAPI ServiceStartAutomatic(DWORD argc, LPTSTR *argv); +VOID WINAPI ServiceStartInteractiveOwn(DWORD argc, LPTSTR *argv); VOID WINAPI ServiceStartInteractive(DWORD argc, LPTSTR *argv); int openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist);