diff mbox series

[Openvpn-devel] Fix some more wrong defines in config-msvc.h

Message ID 20211015175330.15760-1-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Fix some more wrong defines in config-msvc.h |

Commit Message

Selva Nair Oct. 15, 2021, 6:53 a.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Not sure where these came from, but here goes:

S_IRUSR = 0 --> _S_IREAD
S_IWUSR = 0 --> _S_IWRITE

ENABLE_DEBUG is on, but I do not think we want it in production build.
Changed to ENABLE_DEBUG 0

S_IRGRP is not defined but seems to be used. I have added it, remove if
not required.
This define is based on mingw and matches MS docs on <filesystem>
(https://docs.microsoft.com/en-us/cpp/standard-library/filesystem-enumerations?view=msvc-160)

Should fix Trac #1430 but untested (I do not have a setup at hand for msvc build).
---
 config-msvc.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Selva Nair Oct. 15, 2021, 7:50 a.m. UTC | #1 
On Fri, Oct 15, 2021 at 1:53 PM <selva.nair@gmail.com> wrote:

> From: Selva Nair <selva.nair@gmail.com>
>
> Not sure where these came from, but here goes:
>
> S_IRUSR = 0 --> _S_IREAD
> S_IWUSR = 0 --> _S_IWRITE
>
> ENABLE_DEBUG is on, but I do not think we want it in production build.
> Changed to ENABLE_DEBUG 0
>
> S_IRGRP is not defined but seems to be used. I have added it, remove if
> not required.
> This define is based on mingw and matches MS docs on <filesystem>
> (
> https://docs.microsoft.com/en-us/cpp/standard-library/filesystem-enumerations?view=msvc-160
> )
>
> Should fix Trac #1430 but untested (I do not have a setup at hand for msvc
> build).
>

The patch for master doesn't apply cleantly to 2.5. Please use the next one
marked "for 2.5" on the release branch.

Selva
<div dir="ltr"><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 15, 2021 at 1:53 PM &lt;<a href="mailto:selva.nair@gmail.com">selva.nair@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Selva Nair &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;<br>
<br>
Not sure where these came from, but here goes:<br>
<br>
S_IRUSR = 0 --&gt; _S_IREAD<br>
S_IWUSR = 0 --&gt; _S_IWRITE<br>
<br>
ENABLE_DEBUG is on, but I do not think we want it in production build.<br>
Changed to ENABLE_DEBUG 0<br>
<br>
S_IRGRP is not defined but seems to be used. I have added it, remove if<br>
not required.<br>
This define is based on mingw and matches MS docs on &lt;filesystem&gt;<br>
(<a href="https://docs.microsoft.com/en-us/cpp/standard-library/filesystem-enumerations?view=msvc-160" rel="noreferrer" target="_blank">https://docs.microsoft.com/en-us/cpp/standard-library/filesystem-enumerations?view=msvc-160</a>)<br>
<br>
Should fix Trac #1430 but untested (I do not have a setup at hand for msvc build).<br></blockquote><div><br></div><div>The patch for master doesn&#39;t apply cleantly to 2.5. Please use the next one marked &quot;for 2.5&quot; on the release branch.<div><br></div><div>Selva</div></div></div></div>
Lev Stipakov Oct. 17, 2021, 8:09 p.m. UTC | #2 
Hi,

> Not sure where these came from, but here goes:

According to git blame, those dates back to 2012:

https://github.com/OpenVPN/openvpn/commit/4b1a82db0975088dbfe69c2a97f0a96ef972b2a4#diff-f17c6f8c8df2b2934b574e407cbde107338969c6898ce8e8965dc289ae979fc6R97

> S_IRUSR = 0 --> _S_IREAD
> S_IWUSR = 0 --> _S_IWRITE

MSFT docs says that:

"If a value other than some combination of _S_IREAD and _S_IWRITE is
specified for pmode—even if it would specify a valid pmode in another
operating system—or if any value other than the allowed oflag values
is specified, the function generates an assertion in Debug mode and
invokes the invalid parameter handler, as described in Parameter
Validation."

I tried Debug build with --status but haven't seen any assertions.
Anyway, it is clear that passing 0 was incorrect.

> Should fix Trac #1430 but untested (I do not have a setup at hand for msvc build).

If you push your branch to GitHub, it will kick off GitHub Actions
build which produces MSVC-built artifacts.

Built and tested locally, works as expected - status file is created
without "readonly" attribute.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Lev Stipakov Oct. 17, 2021, 8:18 p.m. UTC | #3 
I am sorry, I need to take the ack back. Probably cron2 can fix it in place?

-#define ENABLE_DEBUG 1
+#define ENABLE_DEBUG 0

This doesn't really undef ENABLE_DEBUG. Can we just remove "-#define
ENABLE_DEBUG 1" ?

--
-Lev
Gert Doering Oct. 18, 2021, 10:46 p.m. UTC | #4 
Thanks.  As with the 2.5 patch, removed the DEBUG #define, and changed
commit message to just refer to trac with "Trac: #1430"

Your patch has been applied to the master branch.

commit 077445d0d82128dc90e50043546d4a2d8647eb9c
Author: Selva Nair
Date:   Fri Oct 15 13:53:30 2021 -0400

     Fix some more wrong defines in config-msvc.h

     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20211015175330.15760-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22942.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/config-msvc.h b/config-msvc.h
index b28d8742..301e7ebc 100644
--- a/config-msvc.h
+++ b/config-msvc.h
@@ -4,7 +4,7 @@ 
 
 #define ENABLE_PF 1
 #define ENABLE_CRYPTO_OPENSSL 1
-#define ENABLE_DEBUG 1
+#define ENABLE_DEBUG 0
 #define ENABLE_FRAGMENT 1
 #define ENABLE_HTTP_PROXY 1
 #define ENABLE_LZO 1
@@ -67,8 +67,9 @@ 
 #define in_addr_t uint32_t
 #define ssize_t SSIZE_T
 
-#define S_IRUSR 0
-#define S_IWUSR 0
+#define S_IRUSR _S_IREAD
+#define S_IWUSR _S_IWRITE
+#define S_IRGRP (S_IRUSR >> 3)
 #define R_OK 4
 #define W_OK 2
 #define X_OK 1