Re: [Openvpn-devel] [PATCH v2] openvpnserv: Add support for multi-instances

[Simon R. <si...@ro...>]

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