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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
