Patchwork [libgfortran] Pass mode in "open" for O_CREAT and on VxWorks

login
register
mail settings
Submitter Tobias Burnus
Date May 15, 2012, 9:03 a.m.
Message ID <4FB21BCC.5000305@net-b.de>
Download mbox | patch
Permalink /patch/159262/
State New
Headers show

Comments

Tobias Burnus - May 15, 2012, 9:03 a.m.
Dear all,

the motivation for the following is rbmj's patch for libstdc++ on VxWorks,
cf. http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00073.html

Note that gfortan is used on VxWorks as the following email proves:
http://gcc.gnu.org/ml/fortran/2012-02/msg00115.html
(VxWorks is a real-time embedded operating system:
  https://en.wikipedia.org/wiki/VxWorks)


Seemingly, VxWorks differs from POSIX. POSIX [1] has
   int open(const char *path, int oflag, ... );
and requires only with O_CREAT as third argument at mode_t. By contrast,
VxWorks has the following prototype [2]
   int open (const char * name, int flags, int mode)
where "mode" is not optional and the function is not variadic.


This patch does now always passes a mode on VxWorks; I assume that the mode
is ignored if there is no O_CREAT. That part of the code gets only 
accessed if there is no "access" function; looking at [2], that seems to 
be the case for VxWorks.


Additionally, in the fall-back implementation of "mkstemp", "open" with
O_CREAT is used but no mode has been passed. I added 0600 unconditionally.


The patch was build on x86-64-linux, but I only expect an effect on VxWorks
and on systems without mkstemp.
OK for the trunk? (Is there a need for backporting?)

Tobias

[1] http://pubs.opengroup.org/onlinepubs/000095399/functions/open.html
[2] 
http://www-kryo.desy.de/documents/vxWorks/V5.5/vxworks/ref/ioLib.html#open
Janne Blomqvist - May 16, 2012, 6:45 a.m.
On Tue, May 15, 2012 at 12:03 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Dear all,
>
> the motivation for the following is rbmj's patch for libstdc++ on VxWorks,
> cf. http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00073.html
>
> Note that gfortan is used on VxWorks as the following email proves:
> http://gcc.gnu.org/ml/fortran/2012-02/msg00115.html
> (VxWorks is a real-time embedded operating system:
>  https://en.wikipedia.org/wiki/VxWorks)
>
>
> Seemingly, VxWorks differs from POSIX. POSIX [1] has
>  int open(const char *path, int oflag, ... );
> and requires only with O_CREAT as third argument at mode_t. By contrast,
> VxWorks has the following prototype [2]
>  int open (const char * name, int flags, int mode)
> where "mode" is not optional and the function is not variadic.
>
>
> This patch does now always passes a mode on VxWorks; I assume that the mode
> is ignored if there is no O_CREAT. That part of the code gets only accessed
> if there is no "access" function; looking at [2], that seems to be the case
> for VxWorks.

IMHO it would be cleaner if you instead somewhere in the beginning of unix.c did

#ifdef __VXWORKS__
/* open is not a variadic function on vxworks (or something...) */
#define open(path, flags) open(path, flags, 0666)
#endif

Untested, but AFAICS it should work (unless I'm missing something wrt
macro expansion, which of course is quite possible).

> Additionally, in the fall-back implementation of "mkstemp", "open" with
> O_CREAT is used but no mode has been passed. I added 0600 unconditionally.

Good catch. This part is ok as is.

> The patch was build on x86-64-linux, but I only expect an effect on VxWorks
> and on systems without mkstemp.
> OK for the trunk? (Is there a need for backporting?)

Ok with the above change. I suspect there isn't that much demand for
gfortran on vxworks, but the patch isn't intrusive either so if you
want to backport it go ahead.
Tobias Burnus - May 16, 2012, 10:03 a.m.
Dear Janne,

On 05/16/2012 08:45 AM, Janne Blomqvist wrote:
> IMHO it would be cleaner if you instead somewhere in the beginning of unix.c did
>
> #ifdef __VXWORKS__
> /* open is not a variadic function on vxworks (or something...) */
> #define open(path, flags) open(path, flags, 0666)
> #endif
>
> Untested, but AFAICS it should work (unless I'm missing something wrt
> macro expansion, which of course is quite possible).

Testing shows that CPP does not like it:

$ cpp
#define a(x,y) a(x,y,0666)
a(1,2)
a(1,2,3)
# 1 "<stdin>"
# 1 "<command-line>"
# 1 "<stdin>"

a(1,2,0666)
<stdin>:3:8: error: macro "a" passed 3 arguments, but takes just 2
a


>> The patch was build on x86-64-linux, but I only expect an effect on VxWorks
>> and on systems without mkstemp.
>> OK for the trunk? (Is there a need for backporting?)
> Ok with the above change. I suspect there isn't that much demand for
> gfortran on vxworks, but the patch isn't intrusive either so if you
> want to backport it go ahead.

There might be not much demand, but it *is* being used on VxWorks, thus, 
it potentially affects real users.

However, I was told that VxWorks 6.6 *does* have access() [1] and it is 
defined in unistd.h of 6.3. (Recall that fallback_access does not get 
complied if HAS_ACCESS is true.) He didn't know whether it is available 
in all versions of VxWorks, however. Additionally, he has heard about 
gfortran issues (but has no experience with them); and he promised to 
compile everything to see whether everything works.

[1] 
http://www-ad.fnal.gov/controls/micro_p/manuals/vxworks_application_api_reference_6.6.pdf 



In light of the CPP issue and the HAS_ACCESAS: Shall I only commit the 
second part or also the VxWorks part?

Tobias
Janne Blomqvist - May 16, 2012, 11:26 a.m.
On Wed, May 16, 2012 at 1:03 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Dear Janne,
>
>
> On 05/16/2012 08:45 AM, Janne Blomqvist wrote:
>>
>> IMHO it would be cleaner if you instead somewhere in the beginning of
>> unix.c did
>>
>> #ifdef __VXWORKS__
>> /* open is not a variadic function on vxworks (or something...) */
>> #define open(path, flags) open(path, flags, 0666)
>> #endif
>>
>> Untested, but AFAICS it should work (unless I'm missing something wrt
>> macro expansion, which of course is quite possible).
>
>
> Testing shows that CPP does not like it:
>
> $ cpp
> #define a(x,y) a(x,y,0666)
> a(1,2)
> a(1,2,3)
> # 1 "<stdin>"
> # 1 "<command-line>"
> # 1 "<stdin>"
>
> a(1,2,0666)
> <stdin>:3:8: error: macro "a" passed 3 arguments, but takes just 2

Ah, bummer. We have something roughly similiar for snprintf (see
libgfortran.h), but it seems that it works slightly differently due to
using a variadic macro etc. So it seems this idea will not work,
sorry.

> However, I was told that VxWorks 6.6 *does* have access() [1] and it is
> defined in unistd.h of 6.3. (Recall that fallback_access does not get
> complied if HAS_ACCESS is true.) He didn't know whether it is available in
> all versions of VxWorks, however. Additionally, he has heard about gfortran
> issues (but has no experience with them); and he promised to compile
> everything to see whether everything works.
>
> [1]
> http://www-ad.fnal.gov/controls/micro_p/manuals/vxworks_application_api_reference_6.6.pdf

Interestingly, according to that document open() also has the POSIXly
correct varargs prototype. So apparently the old 3-argument prototype
was used only in some older vxworks version? Since the document above
is from 2007, one wonders how long ago did the prototype change, and
how much new development is done on these older vxworks versions?

> In light of the CPP issue and the HAS_ACCESAS: Shall I only commit the
> second part or also the VxWorks part?

IMHO lets do only the second part now, and wait for someone with
vxworks experience to pop in and explain what changes are necessary,
and which vxworks versions are worth considering to support.
rbmj - May 16, 2012, 12:06 p.m.
On 05/16/2012 07:26 AM, Janne Blomqvist wrote:
> On Wed, May 16, 2012 at 1:03 PM, Tobias Burnus<burnus@net-b.de>  wrote:
>
>
> On 05/16/2012 08:45 AM, Janne Blomqvist wrote:
>>> IMHO it would be cleaner if you instead somewhere in the beginning of
>>> unix.c did
>>>
>>> #ifdef __VXWORKS__
>>> /* open is not a variadic function on vxworks (or something...) */
>>> #define open(path, flags) open(path, flags, 0666)
>>> #endif
>>>
>>> Untested, but AFAICS it should work (unless I'm missing something wrt
>>> macro expansion, which of course is quite possible).
>>
>> Testing shows that CPP does not like it:
>>
>> $ cpp
>> #define a(x,y) a(x,y,0666)
>> a(1,2)
>> a(1,2,3)
>> # 1 "<stdin>"
>> # 1 "<command-line>"
>> # 1 "<stdin>"
>>
>> a(1,2,0666)
>> <stdin>:3:8: error: macro "a" passed 3 arguments, but takes just 2
> Ah, bummer. We have something roughly similiar for snprintf (see
> libgfortran.h), but it seems that it works slightly differently due to
> using a variadic macro etc. So it seems this idea will not work,
> sorry.
Actually, this works for me:

$ cat test.c
#include <stdio.h>

void a(int b, int c, int d) {
    printf("%i\t%i\t%i", b, c, d);
}

#define a(b, c) (a)(b, c, 3)

int main() {
    a(1, 2);
    return 0;
}
$ gcc test.c -o a.out
$ ./a.out
1       2       3

The significance is in the parentheses in the macro definition.
>> However, I was told that VxWorks 6.6 *does* have access() [1] and it is
>> defined in unistd.h of 6.3. (Recall that fallback_access does not get
>> complied if HAS_ACCESS is true.) He didn't know whether it is 
>> available in
>> all versions of VxWorks, however. Additionally, he has heard about 
>> gfortran
>> issues (but has no experience with them); and he promised to compile
>> everything to see whether everything works.
>>
>> [1]
>> http://www-ad.fnal.gov/controls/micro_p/manuals/vxworks_application_api_reference_6.6.pdf 
>>
> Interestingly, according to that document open() also has the POSIXly
> correct varargs prototype. So apparently the old 3-argument prototype
> was used only in some older vxworks version? Since the document above
> is from 2007, one wonders how long ago did the prototype change, and
> how much new development is done on these older vxworks versions?
>
>> In light of the CPP issue and the HAS_ACCESAS: Shall I only commit the
>> second part or also the VxWorks part?
> IMHO lets do only the second part now, and wait for someone with
> vxworks experience to pop in and explain what changes are necessary,
> and which vxworks versions are worth considering to support.
>
I'm using the VxWorks 6.3 headers.  The reason for this is that these 
headers are available at [1], and that this is the version of VxWorks 
used for FRC.  I can also say that these headers do NOT contain the 
POSIX-ly correct version of open().  Additionally, they do NOT have the 
POSIX-ly correct version of ioctl(), so something like the following is 
needed:

#define ioctl(a, b, c) (ioctl)(a, b, (int)c)

To explain FRC- I'm a high school student on a FIRST Robotics 
Competition team working on getting a full gcc4 toolchain, as the 
provided toolchain is gcc 3.4 and windows binaries .  Please don't hold 
my age/experience (or lack thereof) against me.  So if you're looking 
for users, there's probably around 100,000 of us high school students in 
FRC around the world

[1]: ftp://ftp.ni.com/pub/devzone/tut/updated_vxworks63gccdist.zip

Robert Mason
rbmj - May 16, 2012, 2:47 p.m.
On 05/16/2012 08:06 AM, rbmj wrote:
> On 05/16/2012 07:26 AM, Janne Blomqvist wrote:
>> On Wed, May 16, 2012 at 1:03 PM, Tobias Burnus<burnus@net-b.de>  wrote:
>>
>>
>> On 05/16/2012 08:45 AM, Janne Blomqvist wrote:
>>>> IMHO it would be cleaner if you instead somewhere in the beginning of
>>>> unix.c did
>>>>
>>>> #ifdef __VXWORKS__
>>>> /* open is not a variadic function on vxworks (or something...) */
>>>> #define open(path, flags) open(path, flags, 0666)
>>>> #endif
>>>>
>>>> Untested, but AFAICS it should work (unless I'm missing something wrt
>>>> macro expansion, which of course is quite possible).
>>>
>>> Testing shows that CPP does not like it:
>>>
>>> $ cpp
>>> #define a(x,y) a(x,y,0666)
>>> a(1,2)
>>> a(1,2,3)
>>> # 1 "<stdin>"
>>> # 1 "<command-line>"
>>> # 1 "<stdin>"
>>>
>>> a(1,2,0666)
>>> <stdin>:3:8: error: macro "a" passed 3 arguments, but takes just 2
>> Ah, bummer. We have something roughly similiar for snprintf (see
>> libgfortran.h), but it seems that it works slightly differently due to
>> using a variadic macro etc. So it seems this idea will not work,
>> sorry.
> Actually, this works for me:
>
> $ cat test.c
> #include <stdio.h>
>
> void a(int b, int c, int d) {
>    printf("%i\t%i\t%i", b, c, d);
> }
>
> #define a(b, c) (a)(b, c, 3)
>
> int main() {
>    a(1, 2);
>    return 0;
> }
> $ gcc test.c -o a.out
> $ ./a.out
> 1       2       3
Just realized that mine doesn't work either.  Sorry, wasn't looking 
closely enough :-(  The closest one can get is an inline overload, but 
that requires c++.

On a side note, isn't this what autoconf is designed for?

Robert Mason

Patch

2012-05-15  Tobias Burnus  <burnus@net-b.de>

	* io/unix.c (fallback_access): Pass mode to "open" on
	VxWorks.
	(tempfile_open): Pass mode to "open" for O_CREAT

diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index c81163f..b5dca8e 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -154,11 +154,21 @@  fallback_access (const char *path, int mode)
 {
   int fd;
 
+  /* Note that POSIX only requires the mode argument with O_CREAT
+     and "open" is variadic while vxWorks always requires the argument.  */
+#ifdef __VXWORKS__
+  if ((mode & R_OK) && (fd = open (path, O_RDONLY, 0666)) < 0)
+#else
   if ((mode & R_OK) && (fd = open (path, O_RDONLY)) < 0)
+#endif
     return -1;
   close (fd);
 
+#ifdef __VXWORKS__
+  if ((mode & W_OK) && (fd = open (path, O_WRONLY, 0666)) < 0)
+#else
   if ((mode & W_OK) && (fd = open (path, O_WRONLY)) < 0)
+#endif
     return -1;
   close (fd);
 
@@ -1099,9 +1109,9 @@  tempfile_open (const char *tempdir, char **fname)
 
 #if defined(HAVE_CRLF) && defined(O_BINARY)
       fd = open (template, O_RDWR | O_CREAT | O_EXCL | O_BINARY,
-		 S_IRUSR | S_IWUSR);
+		 S_IRUSR | S_IWUSR, 0600);
 #else
-      fd = open (template, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+      fd = open (template, O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR, 0600);
 #endif
     }
   while (fd == -1 && errno == EEXIST);