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

[Selva <sel...@gm...>]

Hi,

On Sun, Nov 5, 2017 at 5:37 PM, Simon Rozman <si...@ro...> 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