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
------------------------------------------------------------------------------
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
