define access mode flag X_OK as 0 on Windows

The _access and _waccess functions in Windows don't know about X_OK (1). If you pass an uneven mode flag the C runtime's default invalid parameter handler ends execution of openvpn.

Signed-off-by Heiko Hund <[email protected]...> --- win/config.h.in

| 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 7 lines · Heiko Hund

The _access and _waccess functions in Windows don't know about X_OK (1). If you pass an uneven mode flag the C runtime's default invalid parameter handler ends execution of openvpn.

Signed-off-by Heiko Hund <[email protected]...> --- win/config.h.in

|    2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

ACK and applied to master branch on -testing and -stable trees.

commit 4ebc587eab73e03ef64d344a5707d84e7f8d875a
Author: Heiko Hund <[email protected]...>
Date:   Sat Feb 18 20:44:12 2012 +0100

   define access mode flag X_OK as 0 on Windows

Signed-off-by Heiko Hund <[email protected]...>

Acked-by David Sommerseth <[email protected]...>

Signed-off-by David Sommerseth <[email protected]...>

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9CHC8ACgkQDC186MBRfrrHegCfUMxu2lHG4Rso8cw1qt7eLEZN OgAAn0cAwDCQKUq5MLJd6WAZJeVsdyje
=2vak
-----END PGP SIGNATURE-----

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

This X_OK is standard define.

It exists in mingw as 1.

OpenVPN should not check execute if unsupported by OS, not changing standard constants.
Where exactly is the problem?

Defining them makes code more readable, but without X_OK being 0 on Windows openvpn crashes. See my commit message again and read the MSDN page about _access at http://msdn.microsoft.com/en-us/library/1w06ktdy%28v=vs.100%29.aspx
Hence the change for the MSVC buildsystem only. Mingw handles the situation internally already.

Hope that made it clear.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

options.c

Quoted 12 lines · Alon Bar-Lev

Reported by Heiko Hund. Reference [1].

Replaces 4ebc587eab73e03ef64d344a5707d84e7f8d875a

[1]
http://msdn.microsoft.com/en-us/library/1w06ktdy%28v=vs.100%29.aspx

Signed-off-by Alon Bar-Lev <[email protected]...> --- options.c

|   38 ++++++++++++++++++++++---------------- win/config.h.in |    2
+- 2 files changed, 23 insertions(+), 17 deletions(-)

options.c +0 -0

I'm not convinced about this at all.  It *is* the proper way to do it, from an intellectual point of view.  But this complicates the reading of the code in places where I don't see it really improve any thing else - except of being pedantically correct.

The reason for this argument is that the following lines of code is in the source tree already:

(win/config.h.in)
#ifndef X_OK
#define X_OK 0
#endif

This is *well contained* inside the build tree which MSVS depends on. This build-platform does not define these macros at all.  [RWX]_OK are all defined in unistd.h, which is non-existent in non-POSIX compliant environment (basically what MSVS falls in under).

So my point of view is that win/config.h.in covers the definition of the required macros which MSVS is lacking.  Of course, it can be argued that such declarations should be in compat.h - as it a compatibility issue. For me that's a slightly different discussion.

The OpenVPN code base is primarily written for the POSIX platform.  MSVS is not a really POSIX friendly platform and mingw is somewhere in between
- - but seems to be far more POSIX oriented than MSVS.  We should focus on keeping the POSIX part of OpenVPN as pure as possible, from a POSIX point of view.  And to be fair, the compat.[ch] files exists purely because of the issues we have with MSVS.

Bottom line is that I'm NACKing this patch, as it complicates things more than make things more clear.  I do admit it is a more correct approach in its principles, but I don't like the complication it implies by "recasting" well defined POSIX macros.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9Cb38ACgkQDC186MBRfrqKhwCghAxMG/X+xKZdiy6gU9FR8Yhf n7oAnjxkrDDjpQngfM1TwTZgLkKVlmdH
=Vupy
-----END PGP SIGNATURE-----

David,

Again,
This has nothing to do with POSIX.
mingw *DOES* define this constant in *WINDOWS*.

We do not want to have a different constants (and different values) at the MSVC and mingw build.
Build should be consistent when possible, but not override environment SDK. Anyway, If this will not enter here it will enter my rewrite... I thought it is better to do this sooner than later.

The definition of PLATFORM_X_OK will be moved to autoconf.

Alon.

Quoted 64 lines · David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

options.c

Quoted 12 lines · Alon Bar-Lev

Reported by Heiko Hund. Reference [1].

Replaces 4ebc587eab73e03ef64d344a5707d84e7f8d875a

[1]
http://msdn.microsoft.com/en-us/library/1w06ktdy%28v=vs.100%29.aspx

Signed-off-by Alon Bar-Lev <[email protected]...> --- options.c

|   38 ++++++++++++++++++++++---------------- win/config.h.in |    2
+- 2 files changed, 23 insertions(+), 17 deletions(-)

options.c +0 -0

I'm not convinced about this at all.  It *is* the proper way to do it, from an intellectual point of view.  But this complicates the reading of the code in places where I don't see it really improve any thing else - except of being pedantically correct.

The reason for this argument is that the following lines of code is in the source tree already:

(win/config.h.in)
#ifndef X_OK
#define X_OK 0
#endif

This is *well contained* inside the build tree which MSVS depends on. This build-platform does not define these macros at all.  [RWX]_OK are all defined in unistd.h, which is non-existent in non-POSIX compliant environment (basically what MSVS falls in under).

So my point of view is that win/config.h.in covers the definition of the required macros which MSVS is lacking.  Of course, it can be argued that such declarations should be in compat.h - as it a compatibility issue. For me that's a slightly different discussion.

The OpenVPN code base is primarily written for the POSIX platform.  MSVS is not a really POSIX friendly platform and mingw is somewhere in between
- - but seems to be far more POSIX oriented than MSVS.  We should focus on keeping the POSIX part of OpenVPN as pure as possible, from a POSIX point of view.  And to be fair, the compat.[ch] files exists purely because of the issues we have with MSVS.

Bottom line is that I'm NACKing this patch, as it complicates things more than make things more clear.  I do admit it is a more correct approach in its principles, but I don't like the complication it implies by "recasting" well defined POSIX macros.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9Cb38ACgkQDC186MBRfrqKhwCghAxMG/X+xKZdiy6gU9FR8Yhf n7oAnjxkrDDjpQngfM1TwTZgLkKVlmdH
=Vupy
-----END PGP SIGNATURE-----

Another option is to use X_OK and declare a macro PLATFORM_MASK_MODE(m)... whatever you think more readable.

Hi,

Quoted 2 lines · Alon Bar-Lev

Another option is to use X_OK and declare a macro PLATFORM_MASK_MODE(m)... whatever you think more readable.

This is something I would like better, as it localizes the change to one place.

gert
--
USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

It still touches the same places...

The difference is the usage:
---
R_OK|PLATFORM_X_OK
---
or:
---
PLATFORM_MODE_MASK(R_OK | X_OK)
---

Hi,

Quoted 10 lines · Alon Bar-Lev

It still touches the same places...

The difference is the usage:
---
R_OK|PLATFORM_X_OK
---
or:
---
PLATFORM_MODE_MASK(R_OK | X_OK)
---

Nah, that's not the way it should be.

Given that we currently use X_OK only in options.c, and have MANY lines of the form

   errs |= check_file_access (CHKACC_FILE, options->crl_file, R_OK|X_OK, "--crl-verify directory");

and only a single place where it will actually call access() (or _access()), anything that makes all these lines harder to read is making the code harder to read.

This code doesn't want or need to know anything about system dependencies, that belongs inside check_file_access() - and there you have only a few access() calls that need to do PLATFORM_MODE_MASK().

Or even better, do it in the openvpn_access() wrapper, in the #ifdef WIN32 code block.  That is the place where the WIN32 pain is, and nobody else needs to bother.

int
openvpn_access (const char *path, int mode)
{
#ifdef WIN32
 struct gc_arena gc = gc_new ();
 int ret = _waccess (wide_string (path, &gc), (mode & ~X_OK) ); gc_free (&gc);
 return ret;
#else
 return access (path, mode);
#endif
}

gert

--
USENET is *not* the non-clickable part of WWW! //www.muc.de/~gert/ Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

This reverts commit 4ebc587e. Instead of defining X_OK 0 for MSVC, this undefined mode value is now simply masked out close to where it causes the trouble; in the call to _waccess() as suggested during the discussion on the development mailing list:
http://thread.gmane.org/gmane.network.openvpn.devel/5452

Signed-off-by Heiko Hund <[email protected]...>

David,

Again, This has nothing to do with POSIX. mingw *DOES* define this constant in *WINDOWS*.

We do not want to have a different constants (and different values) at the MSVC and mingw build.

Build should be consistent when possible, but not override environment SDK.

Anyway, If this will not enter here it will enter my rewrite... I thought it is better to do this sooner than later.

The definition of PLATFORM_X_OK will be moved to autoconf.

But remember that all your patches *will* go through review as for everyone else in the community.  That means either *1)* patch reviews on the mailing list, or *2)* reviewed through a sprint review in a developers meeting on the IRC.  You are free to choose between which approach fits you best.  But I will not accept patches into the tree unless it has been clearly ACKed publicly.

Please start go over my patches in github[1] and mark your comments, this way we know if it was a total waste of time.

Alon.

[1] https://github.com/alonbl/openvpn

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 6 lines · Alon Bar-Lev

Please start go over my patches in github[1] and mark your comments, this way we know if it was a total waste of time.

Alon.

[1] https://github.com/alonbl/openvpn

Alon,

If you want to have the review on the mailing list, then I want to see those patches nicely formatted as patches here - which can easily be sent as with a single git send-email command.  This way it is far easier to document the review process.

The alternative, as you know, is an IRC sprint where we can go through commit by commit on a web site.  Doing it this way we document the review in an e-mail with a summary of the discussions plus the chatlog itself to the mailing list.  This approach was used for the PolarSSL patches.

kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9C0R0ACgkQDC186MBRfrqzigCfcfGrzNbFmZvfNNUix4YNgXpt ONUAnRk6N31HU/Jf5+pvyk4BrVtqbZAe
=PzyD
-----END PGP SIGNATURE-----

Quoted 6 lines · David Sommerseth

But remember that all your patches *will* go through review as for everyone else in the community. That means either *1)* patch reviews on the mailing list, or *2)* reviewed through a sprint review in a developers meeting on the IRC. You are free to choose between which approach fits you best. But I will not accept patches into the tree unless it has been clearly ACKed publicly.

David,

I see I wasted three days of work.

You agreed that you have a problem "Huston we have a problem, the build process of OpenVPN causes maintenance overhead".

Nobody in this community maintains the build system so it became so complex and none intuitive that it is unusable.

You won't be able to review all my changes as it is a revolution not evolution, I warned you guys ahead! It will touch almost all the sources.