Re: [Openvpn-devel] [PATCH applied] Re: Extend FindSystemInfo custom action to detect OpenVPNService state
Hi,
On Sun, Jan 20, 2019 at 01:19:50PM +, Simon Rozman wrote:
> Now, I could leave that semicolon in place to keep IntelliSense false
> warning away, but looking at the code I asked myself a question: "If my code
> is so convoluted it produces a false warning to a bot, how convoluted it
> must be for a human to understand?" False warnings are usually sign of
> overcomplex or atypical code design. I have refactored the code.
Thanks for the explanation, I wasn't aware that there is a special case
where a semicolon is required after a label. Even if only MSVC :-)
But second half is even better ;-)
> > Something else might not be obvious to the reader:
> >
> > +if (pQsc->dwStartType <= SERVICE_AUTO_START)
> > +{
> >
> > so what does "lesser than AUTO_START" mean? manual start? no start at
> > all? Comparing for "lesser or equal" with something enum-like might
> > warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */
> > or so... (yes, I can google this, but still).
>
> This is a very generic test "Is driver or service set to start automatically
> in any phase of the system launch?" on the Windows.
Ah. So "anyone who has any clue about windows services would know and
understand" - good enough. We need more clueful windows people to review
windows patches :-)
(Selva is hiding somewhere)
> I've added a comment with a brief explanation.
>
> A patch will follow.
Thanks!
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH applied] Re: Extend FindSystemInfo custom action to detect OpenVPNService state
Hi,
> +finish_QueryServiceStatusEx:;
> +
> +// Service is not started. Is it set to auto-start?
> +// MSDN describes the maximum buffer size for QueryServiceConfig()
> to be 8kB.
> +// This is small enough to fit on stack.
>
> .. there shouldn't be a ";" after a label, and no C++ comments... but I
> assume the uncrustify patch coming next will fix that...
The sentence following the finish_QueryServiceStatusEx label is a local
variable declaration. While MSVC and MinGW are fine with or without
semicolon, the Visual Studio 2017 IDE's IntelliSense is complaining with
"E1072: a declaration cannot have a label".
Now, I could leave that semicolon in place to keep IntelliSense false
warning away, but looking at the code I asked myself a question: "If my code
is so convoluted it produces a false warning to a bot, how convoluted it
must be for a human to understand?" False warnings are usually sign of
overcomplex or atypical code design. I have refactored the code.
>
> Something else might not be obvious to the reader:
>
> +if (pQsc->dwStartType <= SERVICE_AUTO_START)
> +{
>
> so what does "lesser than AUTO_START" mean? manual start? no start at
> all? Comparing for "lesser or equal" with something enum-like might
> warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */
> or so... (yes, I can google this, but still).
This is a very generic test "Is driver or service set to start automatically
in any phase of the system launch?" on the Windows. I've added a comment
with a brief explanation.
A patch will follow.
Regards,
Simon
smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: Extend FindSystemInfo custom action to detect OpenVPNService state
Acked-by: Gert Doering
This is code I can even understand (to some extent) ;-) - overall it looks
good, but a few style warts sneaked in...
+finish_QueryServiceStatusEx:;
+
+// Service is not started. Is it set to auto-start?
+// MSDN describes the maximum buffer size for QueryServiceConfig() to be
8kB.
+// This is small enough to fit on stack.
.. there shouldn't be a ";" after a label, and no C++ comments... but
I assume the uncrustify patch coming next will fix that...
Something else might not be obvious to the reader:
+if (pQsc->dwStartType <= SERVICE_AUTO_START)
+{
so what does "lesser than AUTO_START" mean? manual start? no start at
all? Comparing for "lesser or equal" with something enum-like might
warrant a comment /* BOOT_START = 0, SYSTEM_START = 1, AUTO_START = 2 */
or so... (yes, I can google this, but still).
Compile tested on Ubuntu 16.04 / mingw.
Your patch has been applied to the master branch.
commit 8148ee9d01de75d0302f224ef499eddfabc6fee2
Author: Simon Rozman
Date: Wed Dec 19 21:26:09 2018 +0100
Extend FindSystemInfo custom action to detect OpenVPNService state
Acked-by: Gert Doering
Message-Id: <20181219202611.2144-2-si...@rozman.si>
URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18039.html
Signed-off-by: Gert Doering
--
kind regards,
Gert Doering
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
