[ace-users] ACE_Process::spawn() file handles

[Christian Fromme]

Hi *,

Is there a way to close inherited file handles or to not inherit file
handles at all after ACE_Process::spawn()?

I've checked and tried most of what looked remotely like it could work
in ACE_Process_Options, but no success.

TIA,
Christian Fromme

[Christian Fromme]

Hello Steve,

thanks for your quick reply.

On 20.10. 11:42, Steve Huston wrote:

> > Is there a way to close inherited file handles or to not inherit
> file
> > handles at all after ACE_Process::spawn()?
>

> Check ACE::daemonize() - it knows how to do this.

Thanks. I think my initial mail was a bit misleading. I'd like to avoid
file decriptor inheritance on the parent side of ACE_Process::spwan().
It currently seems like FD_CLOEXEC for each of the parent's FDs is the only
way to accomplish this.

> Please include the PROBLEM-REPORT-FORM if you still have questions.
> The info you want is very platform-specific and may be ACE
> version-specific as well.

Here is the relevant information from the PRF:

ACE VERSION: ACE-5.5+TAO-1.5

HOST MACHINE and OPERATING SYSTEM: Linux 2.6.X

TARGET MACHINE and OPERATING SYSTEM, if different from HOST:
COMPILER NAME AND VERSION (AND PATCHLEVEL): gcc-Version 3.4.3

Best,
Christian Fromme

[Douglas C. Schmidt]

Hi Christian,

>Thanks. I think my initial mail was a bit misleading. I'd like to avoid
>file decriptor inheritance on the parent side of ACE_Process::spwan().
>It currently seems like FD_CLOEXEC for each of the parent's FDs is the only
>way to accomplish this.

It appears that there was an attempt to account for this in the
ACE_Process_Options::handle_inheritence() methods (BTW, "inheritance"
is spelled wrong in this API), but it doesn't appear that this
capability is actually used for anything. If you'd like to enhance
ACE_Process to use this setting to avoid handle inheritance that would
be great!

Thanks,

Doug

>> Please include the PROBLEM-REPORT-FORM if you still have questions.
>> The info you want is very platform-specific and may be ACE
>> version-specific as well.
>
>Here is the relevant information from the PRF:
>
> ACE VERSION: ACE-5.5+TAO-1.5
>
> HOST MACHINE and OPERATING SYSTEM: Linux 2.6.X
>
> TARGET MACHINE and OPERATING SYSTEM, if different from HOST:
> COMPILER NAME AND VERSION (AND PATCHLEVEL): gcc-Version 3.4.3
>
>Best,
>Christian Fromme

--
Dr. Douglas C. Schmidt Professor and Associate Chair
Electrical Engineering and Computer Science TEL: (615) 343-8197
Vanderbilt University WEB: www.dre.vanderbilt.edu/~schmidt
Nashville, TN 37203 NET: d.sc...@vanderbilt.edu

[Christian Fromme]

On 21.10. 10:43, Douglas C. Schmidt wrote:
>
> >Thanks. I think my initial mail was a bit misleading. I'd like to avoid
> >file decriptor inheritance on the parent side of ACE_Process::spwan().
> >It currently seems like FD_CLOEXEC for each of the parent's FDs is the only
> >way to accomplish this.
>
> It appears that there was an attempt to account for this in the
> ACE_Process_Options::handle_inheritence() methods (BTW, "inheritance"
> is spelled wrong in this API), but it doesn't appear that this
> capability is actually used for anything. If you'd like to enhance
> ACE_Process to use this setting to avoid handle inheritance that would
> be great!

Here is the patch against ACE+TAO+CIAO-5.6.6. I tested this *only* on
Linux 2.6.X (Debian Etch).

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
diff -ru ACE_wrappers/ace/Process.cpp ACE_wrappers_work/ace/Process.cpp
--- ACE_wrappers/ace/Process.cpp 2008-08-04 22:01:17.000000000 +0200
+++ ACE_wrappers_work/ace/Process.cpp 2008-10-22 14:42:54.548146401 +0200
@@ -358,6 +358,14 @@

return this->child_id_;
#else /* ACE_WIN32 */
+ if (!options.handle_inheritence()) {
+ // Set close-on-exec for all FDs except standard handles
+ for (int i = ACE::max_handles () - 1; i >= 0; i--) {
+ if ((i == ACE_STDIN) || (i == ACE_STDOUT) || (i == ACE_STDERR))
+ continue;
+ ACE_OS::fcntl (i, F_SETFD, FD_CLOEXEC);
+ }
+ }
// Fork the new process.
this->child_id_ = ACE::fork (options.process_name (),
options.avoid_zombies ());
@@ -802,6 +810,7 @@
stdin_ (ACE_INVALID_HANDLE),
stdout_ (ACE_INVALID_HANDLE),
stderr_ (ACE_INVALID_HANDLE),
+ handle_inheritence_ (true),
ruid_ ((uid_t) -1),
euid_ ((uid_t) -1),
rgid_ ((uid_t) -1),
diff -ru ACE_wrappers/ace/Process.h ACE_wrappers_work/ace/Process.h
--- ACE_wrappers/ace/Process.h 2008-07-02 00:49:26.000000000 +0200
+++ ACE_wrappers_work/ace/Process.h 2008-10-22 12:14:38.507724900 +0200
@@ -371,6 +371,9 @@
ACE_HANDLE stdout_;
ACE_HANDLE stderr_;

+ /// Default true.
+ bool handle_inheritence_;
+
// = Real & effective user & group id's.
// These should be set to -1 to leave unchanged (default).
uid_t ruid_;
diff -ru ACE_wrappers/ace/Process.inl ACE_wrappers_work/ace/Process.inl
--- ACE_wrappers/ace/Process.inl 2008-03-04 15:51:23.000000000 +0100
+++ ACE_wrappers_work/ace/Process.inl 2008-10-22 10:46:07.715771239 +0200
@@ -131,22 +131,22 @@
ACE_INLINE int
ACE_Process_Options::handle_inheritence (void)
{
-#if defined (ACE_WIN32) && !defined (ACE_HAS_WINCE)
+#if !defined (ACE_HAS_WINCE) && !defined (ACE_VXWORKS) && !defined(ACE_OPENVMS)
return handle_inheritence_;
#else
- ACE_NOTSUP_RETURN (0); // This is a benign error.
-#endif /* ACE_WIN32 && ! ACE_HAS_WINCE */
+ ACE_NOTSUP_RETURN (0); // This is a benign error.
+#endif /* ! ACE_HAS_WINCE && !ACE_VXWORKS && !ACE_OPENVMS */
}

ACE_INLINE void
ACE_Process_Options::handle_inheritence (int hi)
{
-#if defined (ACE_WIN32) && !defined (ACE_HAS_WINCE)
+#if !defined (ACE_HAS_WINCE) && !defined (ACE_VXWORKS) && !defined(ACE_OPENVMS)
handle_inheritence_ = hi;
#else
ACE_UNUSED_ARG (hi);
ACE_NOTSUP;
-#endif /* !ACE_HAS_WINCE */
+#endif /* !ACE_HAS_WINCE && !ACE_VXWORKS && !ACE_OPENVMS */
}

ACE_INLINE int
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

HTH,
Christian

[Douglas C. Schmidt]

Hi, Christian,

I've added your patches. I've also fixed the misspelling of
"handle_inheritence" to be "handle_inheritance". If someone knows how
to add support for enabling/disabling handle inheritance on Windows
please let us know.

Thanks very much,

Doug

> _______________________________________________
> ace-users mailing list
> ace-...@list.isis.vanderbilt.edu
> http://list.isis.vanderbilt.edu/mailman/listinfo/ace-users

[Adam Mitz]

Douglas C. Schmidt wrote:
> Hi, Christian,
>
> I've added your patches. I've also fixed the misspelling of
> "handle_inheritence" to be "handle_inheritance". If someone knows how
> to add support for enabling/disabling handle inheritance on Windows
> please let us know.
>

Hasn't ACE always supported this on Windows? The call to CreateProcess
includes the line:
options.handle_inheritence ()
(Process.cpp line 178 before your commit)

Thanks,
Adam Mitz
Software Engineer
Object Computing, Inc.

[Douglas C. Schmidt]

Hi Adam,

> > I've added your patches. I've also fixed the misspelling of
> > "handle_inheritence" to be "handle_inheritance". If someone knows how
> > to add support for enabling/disabling handle inheritance on Windows
> > please let us know.
> >
>
> Hasn't ACE always supported this on Windows? The call to
> CreateProcess includes the line:
> options.handle_inheritence ()
> (Process.cpp line 178 before your commit)

As far as I can tell, this method doesn't do anything on Windows yet.
Please take a look at the code and let me know if you see where it's
actually used!

Thanks,

Doug

[Adam Mitz]

It's used on line 178 of Process.cpp (unless I'm missing something...)
-- here's some more context:

BOOL fork_result =
ACE_TEXT_CreateProcess (0,
options.command_line_buf (),
options.get_process_attributes (),
options.get_thread_attributes (),
options.handle_inheritence (),
flags,
env_buf, // environment variables
options.working_directory (),
options.startup_info (),
&this->process_info_);

[Douglas C. Schmidt]

Hi Adam,

> It's used on line 178 of Process.cpp (unless I'm missing something...)

Cool, it looks like we're all set now!!

Thanks,

Doug

[Jeff Parsons]

Hi,

> -----Original Message-----
> From: ace-user...@list.isis.vanderbilt.edu [mailto:ace-users-
> bou...@list.isis.vanderbilt.edu] On Behalf Of Douglas C. Schmidt
> Sent: Wednesday, October 22, 2008 9:14 AM
> To: Christian Fromme
> Cc: ace-...@cse.wustl.edu; Christian Fromme
> Subject: Re: [ace-users] ACE_Process::spawn() file handles
>
>
> Hi, Christian,

>
> I've added your patches. I've also fixed the misspelling of
> "handle_inheritence" to be "handle_inheritance". If someone knows how
> to add support for enabling/disabling handle inheritance on Windows
> please let us know.

It looks to me like the Windows case is also handled by these patches.
ACE_Process_Options has 2 mutually exclusive 'handle_inheritance_'
members,
a BOOL for Windows and a bool for all others. The Windows-only member
then gets passed to the native CreateProcess, so the accessor/mutator
pair of methods that were checked in should work for Windows as well.

Jeff

[Christian Fromme]

On 22.10. 09:14, Douglas C. Schmidt wrote:
>
> I've added your patches. I've also fixed the misspelling of
> "handle_inheritence" to be "handle_inheritance". If someone knows how
> to add support for enabling/disabling handle inheritance on Windows
> please let us know.

I think the others replys to this thread already pointed out that things
for Windows already looked well.

> Thanks very much,

Thanks for adding the patch,
Christian

[Johnny Willemsen]

Hi,

Why does OpenVMS/VxWorks did got added here? Seems these accessors now also
should be of type bool?

Johnny

[Christian Fromme]

Because the code in Process.cpp also #defines those out.

Best,
Christian

[Johnny Willemsen]

Him

> > Why does OpenVMS/VxWorks did got added here? Seems these accessors
> now also
> > should be of type bool?
>
> Because the code in Process.cpp also #defines those out.

For such simple accessors I would just set/get the value when the member
variable is there, makes the code much easier to maintain.

Johnny

[Douglas C. Schmidt]

Hi Jeff,

> It looks to me like the Windows case is also handled by these patches.
> ACE_Process_Options has 2 mutually exclusive 'handle_inheritance_'
> members,
> a BOOL for Windows and a bool for all others. The Windows-only member
> then gets passed to the native CreateProcess, so the accessor/mutator
> pair of methods that were checked in should work for Windows as well.

Please take a look at the changes I just commited and see if that works better.

Thanks,

Doug

[Jeff Parsons]

Hi,

That's definitely better, since we now have only one handle_inheritance_
member
for all platforms.

However, on further study of the Windows documentation, I'm not sure we
have
everything we need to get the desired behavior on Windows:

Paraphrasing the docs I read, the bare boolean that gets passed to the
Windows
native CreateProcess determines whether the *inheritable* handles of the
calling
process get inherited by the new process. There is another arg passed to
this
call (ACE passes in the process_attributes_ member), which on Windows is
a struct
that itself has a boolean member called bInheritHandle. If I understand
the docs
correctly, it's this member value that determines whether the new handle
is itself
an 'inheritable' one. So it would seem that both these booleans would
need to
be set before calling CreateProcess, and it doesn't look like ACE is
doing
anything with the process_attributes_ member value, probably passing
NULL. I
guess a test would be in order to sort out the Windows situation - let
me
excuse myself from getting into an "Ossama" ;-) at the outset by saying
that
I definitely don't have time to do this right now.

Hope this helps,

Jeff

> -----Original Message-----
> From: sch...@dre.vanderbilt.edu [mailto:sch...@dre.vanderbilt.edu]
> Sent: Wednesday, October 22, 2008 10:08 AM
> To: Jeff Parsons
> Cc: Christian Fromme; ace-...@cse.wustl.edu; Christian Fromme
> Subject: Re: [ace-users] ACE_Process::spawn() file handles
>
>

[Adam Mitz]

I think what ACE does now is fine. The process_attributes_ just deals
with the process handle returned from CreateProcess(), that's not the
same issue has handle inheritance into the spawned process.

[J.T. Conklin]

Christian Fromme <chri...@ast.dfs.de> writes:
> On 21.10. 10:43, Douglas C. Schmidt wrote:
>> >Thanks. I think my initial mail was a bit misleading. I'd like to
>> >avoid file decriptor inheritance on the parent side of
>> >ACE_Process::spwan(). It currently seems like FD_CLOEXEC for each
>> >of the parent's FDs is the only way to accomplish this.
>>
>> It appears that there was an attempt to account for this in the
>> ACE_Process_Options::handle_inheritence() methods (BTW, "inheritance"
>> is spelled wrong in this API), but it doesn't appear that this
>> capability is actually used for anything. If you'd like to enhance
>> ACE_Process to use this setting to avoid handle inheritance that would
>> be great!
>
> Here is the patch against ACE+TAO+CIAO-5.6.6. I tested this *only* on
> Linux 2.6.X (Debian Etch).

Hi Christian,

It looks like that this changes the close on exec flag for all open
file descriptors globably.

If you had two separate ACE_Process objects, one that needs to inherit
open file descriptors and another that must not; this will fail if the
latter is spawned before the former. As such, I think this may not be
a desirable change. At the very least, I believe this behavior would
need to be clearly documented.

I'm not sure whether this can be fixed by setting the FL_CLOEXEC flag
in the child process after the fork() -- from reading POSIX.1-1990, I
see that "The child process has its own copy of the parent's file
descriptors. Each of the child's descriptors refers to the same open
file description with the cooresponding file descriptor of the
parent", which suggests it should.

--jtc

--
J.T. Conklin

[Christian Fromme]

Hi jtc,

On 22.10. 09:21, J.T. Conklin wrote:
> It looks like that this changes the close on exec flag for all open
> file descriptors globably.
>
> If you had two separate ACE_Process objects, one that needs to inherit
> open file descriptors and another that must not; this will fail if the
> latter is spawned before the former. As such, I think this may not be
> a desirable change. At the very least, I believe this behavior would
> need to be clearly documented.

You're right. I clearly oversaw this, thanks for pointing it out.

> I'm not sure whether this can be fixed by setting the FL_CLOEXEC flag
> in the child process after the fork() -- from reading POSIX.1-1990, I
> see that "The child process has its own copy of the parent's file
> descriptors. Each of the child's descriptors refers to the same open
> file description with the cooresponding file descriptor of the
> parent", which suggests it should.

Looking at the Linux kernel source, it seems like do_execve() from
fs/exec.c closes FDs marked as CLOEXEC through search_binary_handler().
This means setting CLOEXEC for all *after* the fork is fine at least on
Linux. Patch for ace/Process.cpp against the current svn (83381) below.
Tested on Linux.

Thanks,
Christian

--------
--- Process_old.cpp 2008-10-23 08:15:11.505169819 +0200
+++ Process.cpp 2008-10-23 08:17:34.640739994 +0200
@@ -358,14 +358,6 @@

return this->child_id_;
#else /* ACE_WIN32 */

- if (!options.handle_inheritance()) {
- // Set close-on-exec for all FDs except standard handles
- for (int i = ACE::max_handles () - 1; i >= 0; i--) {
- if ((i == ACE_STDIN) || (i == ACE_STDOUT) || (i == ACE_STDERR))
- continue;
- ACE_OS::fcntl (i, F_SETFD, FD_CLOEXEC);
- }
- }

// Fork the new process.
this->child_id_ = ACE::fork (options.process_name (),
options.avoid_zombies ());

@@ -462,6 +454,14 @@
ACE_OS::close (options.get_stdin ());
ACE_OS::close (options.get_stdout ());
ACE_OS::close (options.get_stderr ());

+ if (!options.handle_inheritence()) {
+ // Set close-on-exec for all FDs except standard handles
+ for (int i = ACE::max_handles () - 1; i >= 0; i--) {
+ if ((i == ACE_STDIN) || (i == ACE_STDOUT) || (i == ACE_STDERR))
+ continue;
+ ACE_OS::fcntl (i, F_SETFD, FD_CLOEXEC);
+ }
+ }

// If we must, set the working directory for the child
// process.
--------

[Douglas C. Schmidt]

Thanks Christian,

I've added your latest patch. It would be great if you could
create an ACE_ROOT/tests/Process_Test.cpp (e.g., by modifying the
existing Process_Manager_Test.cpp) to handle this stuff.

Thanks,

Doug

[Christian Fromme]

Hi Doug,

On 23.10. 07:55, Douglas C. Schmidt wrote:
>
> I've added your latest patch.

The patch is not applied in the svn as of revision 83427[0]. The patch I
sent was against the current svn, with the previous patch already
applied. Maybe that was the problem? I still see the CLOEXEC loop above
fork() in Process.cpp, after the latest patch it should be somewhere
below fork().

I hope I am not confusing something here,
Christian

[0]
https://svn.dre.vanderbilt.edu/viewvc/Middleware/trunk/ACE/ace/Process.cpp?view=markup&pathrev=83427

[Douglas C. Schmidt]

Hi Christian,

Please look at

http://www.dre.vanderbilt.edu/~schmidt/DOC_ROOT/ACE/ace/Process.cpp

and send me a patch relative to this that fixes the problems you observed.

Thanks very much,

Doug

[Christian Fromme]

On 23.10. 09:38, Douglas C. Schmidt wrote:
>
> Please look at
>
> http://www.dre.vanderbilt.edu/~schmidt/DOC_ROOT/ACE/ace/Process.cpp
>
> and send me a patch relative to this that fixes the problems you observed.

This version is ok. It seems like the latest revision in your SVN
(83427) that I can see is different from the one in /~schmidt/. Never
mind then and sorry for the inconvenience. ;-)

About integrating a test for this, I don't know if I will have time
for this. If so, I will send you a patch within the next few days.

Best,
Christian

[Douglas C. Schmidt]

Hi Christian,

> A first version of the Process_Test.cpp is attached. Please check if
> anything comes to mind that I have forgotten or overseen.

I've fixed several things and integrated this test into the SVN repo.

> An obvious drawback is that it currently checks for open files through
> '/proc/', which limits its use to UNIX-like systems. Any ideas how
> this could be changed?

I'm not sure how to do this on Windows, but hopefully others know better
than I do. Now that it's integrated into the SVN repo other folks can
work on porting the test.

> Do you also need a patch for the various Makefiles and so on where
> this test needs to get included?

I believe I've added everything we need at this point and commited it to
the SVN repo.

Thanks very much!

Doug

[Jeff Parsons]

Hi,

Is this something that could be added to the monitor framework?
What does this test check?

thanks,

Jeff

> -----Original Message-----
> From: ace-user...@list.isis.vanderbilt.edu [mailto:ace-users-
> bou...@list.isis.vanderbilt.edu] On Behalf Of Douglas C. Schmidt
> Sent: Tuesday, November 04, 2008 9:32 AM
> To: Christian Fromme

> Cc: ace-...@list.isis.vanderbilt.edu
> Subject: Re: [ace-users] ACE_Process::spawn() file handles
>
>

[Douglas C. Schmidt]

Hi Jeff,

> Is this something that could be added to the monitor framework?
> What does this test check?

It checks to make sure that the new POSIX "handle_inheritance()"
implementation works. I'm not surehow the monitor framework can help
here, but if it can that would be great!! Can you please chat with
Christian about this?

Thanks very much,

Doug

[Jeff Parsons]

Hi,

Ok, I see, you're probably right about the monitor framework being
used directly. My original thought was that there are several OS
monitors in there that use /proc on Linux, and corresponding stuff
on Solaris and Windows. At the very least, inspection of that code
might yield some ideas about how to make Christian's test more
portable.

Jeff

> -----Original Message-----
> From: sch...@dre.vanderbilt.edu [mailto:sch...@dre.vanderbilt.edu]
> Sent: Tuesday, November 04, 2008 9:40 AM
> To: Jeff Parsons

[Douglas C. Schmidt]

Hi Jeff,

> Ok, I see, you're probably right about the monitor framework being
> used directly. My original thought was that there are several OS
> monitors in there that use /proc on Linux, and corresponding stuff
> on Solaris and Windows. At the very least, inspection of that code
> might yield some ideas about how to make Christian's test more
> portable.

Yes, it would be cool if we could leverage the monitor implementation
for insights into this stuff!

Thanks,

Doug

[Christian Fromme]

Hi Jeff,

On 04.11. 09:44, Jeff Parsons wrote:

> Ok, I see, you're probably right about the monitor framework being
> used directly. My original thought was that there are several OS
> monitors in there that use /proc on Linux, and corresponding stuff
> on Solaris and Windows. At the very least, inspection of that code
> might yield some ideas about how to make Christian's test more
> portable.

I'd be glad to see this ported to other platforms. The '/proc' approach
was the only idea I had for now, maybe there are other ways to do this
in a more portable way. *If* there is a way, it'd be nice to see
something like ACE_OS::return_open_files() or something similar in the
ACE API. Let me know if I can help.

Best,
Christian

[Douglas C. Schmidt]

Hi Folks,

That sounds good. Jeff, can you please check to see whether this sort
of capability is available via the monitoring framework?

Thanks,

Doug

[Jeff Parsons]

Hi,

Returning a list of open files is definitely not in the monitor
framework. However, you can see that when the monitor framework
does something on Linux by using /proc, it often does the same
thing on Solaris using kstat and on Windows using the PDH (Performance
Data Helper) API. It shouldn't be hard to look in the documentation
of these things and see if there's anything that keeps track of
open files. When I was implementing the monitor framework, I just
used the Help menu in Visual Studio for PDH and Google for kstat.

Jeff

> -----Original Message-----
> From: sch...@dre.vanderbilt.edu [mailto:sch...@dre.vanderbilt.edu]
> Sent: Tuesday, November 04, 2008 9:59 AM
> To: Christian Fromme
> Cc: Jeff Parsons; ace-...@list.isis.vanderbilt.edu
> Subject: Re: [ace-users] ACE_Process::spawn() file handles
>
>

[Simon Massey]

The current Process_test.cpp has several builds on the scoreboard
chokeing.

Can someone please look at:
http://www.dre.vanderbilt.edu/scoreboard/VxWorks6.4_PPC603_IPv6_Release/
2008_11_04_21_37_Full.html#error_1

VxWorks systems (readlink is not a member of ACE_OS)

http://www.dre.vanderbilt.edu/scoreboard/WinXP_Cygwin/2008_11_04_22_19_F
ull.html#error_2
WinXP_cygwin (the above and also open, and getpid is not a member of
ACE_OS)
Also on the FC9_Static_Core system and Fedora_Rawhide_i386 and Redhat
systems.
And Solaris10 systems

It also has problems on WinXP systems:
http://www.dre.vanderbilt.edu/scoreboard/WinXP_VC8_FULL/2008_11_05_02_57
_Full.html#error_3

It also has problems on wchar systems:
http://www.dre.vanderbilt.edu/scoreboard/WinXP_RAD_Studio_2007_Dynamic_D
ebug_Unicode/2008_11_05_08_23_Full.html#error_1

-----Original Message-----
From: ace-user...@list.isis.vanderbilt.edu
[mailto:ace-user...@list.isis.vanderbilt.edu] On Behalf Of
Christian Fromme
Sent: Tuesday, 04 November 2008 15:56
To: Jeff Parsons
Cc: ace-...@list.isis.vanderbilt.edu
Subject: Re: [ace-users] ACE_Process::spawn() file handles

Hi Jeff,

On 04.11. 09:44, Jeff Parsons wrote:

> Ok, I see, you're probably right about the monitor framework being
> used directly. My original thought was that there are several OS
> monitors in there that use /proc on Linux, and corresponding stuff on
> Solaris and Windows. At the very least, inspection of that code might
> yield some ideas about how to make Christian's test more portable.

I'd be glad to see this ported to other platforms. The '/proc' approach
was the only idea I had for now, maybe there are other ways to do this
in a more portable way. *If* there is a way, it'd be nice to see
something like ACE_OS::return_open_files() or something similar in the
ACE API. Let me know if I can help.

Best,
Christian

[Christian Fromme]

Hi Simon,

thanks for this information.

On 05.11. 07:59, Simon Massey wrote:
>
> The current Process_test.cpp has several builds on the scoreboard
> chokeing.
>
> Can someone please look at:
> http://www.dre.vanderbilt.edu/scoreboard/VxWorks6.4_PPC603_IPv6_Release/
> 2008_11_04_21_37_Full.html#error_1
>
> VxWorks systems (readlink is not a member of ACE_OS)
>
> http://www.dre.vanderbilt.edu/scoreboard/WinXP_Cygwin/2008_11_04_22_19_F
> ull.html#error_2
> WinXP_cygwin (the above and also open, and getpid is not a member of
> ACE_OS)
> Also on the FC9_Static_Core system and Fedora_Rawhide_i386 and Redhat
> systems.
> And Solaris10 systems
>
> It also has problems on WinXP systems:
> http://www.dre.vanderbilt.edu/scoreboard/WinXP_VC8_FULL/2008_11_05_02_57
> _Full.html#error_3
>
> It also has problems on wchar systems:
> http://www.dre.vanderbilt.edu/scoreboard/WinXP_RAD_Studio_2007_Dynamic_D
> ebug_Unicode/2008_11_05_08_23_Full.html#error_1

ACE_OS::readlink is not available on some platforms -- ok. We could work
around this by adding "|| defined (ACE_LACKS_READLINK)" to the line
with "#if defined (ACE_LACKS_FORK)" -- so the test won't be used on
platforms without readlink. (Btw: How is /proc/self/fd/* managed on
other platforms apart form Linux? No link to the open file I guess?)

But what puzzles me is that ACE_OS::open and ACE_OS::getpid aren't found
on WinXP_cygwin and others, while the log clearly states that tests like
ARGV_Test.cpp or Log_Msg_Test.cpp compile without error even though the
source I have here says they're also using those functions.

The other errors on wchar systems should be possible to get rid off.

Best,
Christian

[Simon Massey]

I think I've done the wide character bits.

[Adam Mitz]

The new test needs an include for "ace/OS_NS_unistd.h" in order to use
ACE_OS::readlink() and getpid(), and OS_NS_fcntl.h for open(). (Do you
really need open or can it be fopen?)

[Christian Fromme]

Thanks! Will someone with commit access add the fixes directly or do you
need me to write a patch? Fopen is fine as well.

Best,
Christian

[Simon Massey]

The FREEZE is in name only, anyone can still commit to fix these
scoreboard problems. If you need me to add the header however, I can do
that for you.

-----Original Message-----
From: Christian Fromme [mailto:chri...@ast.dfs.de]

[Douglas C. Schmidt]

Hi Christian,

> Thanks! Will someone with commit access add the fixes directly or do
> you need me to write a patch? Fopen is fine as well.

Please grab the latest version from

http://www.dre.vanderbilt.edu/~schmidt/DOC_ROOT/ACE/tests/Process_Test.cpp

and send me the patches relative to that.

Thanks very much,

Doug

[Christian Fromme]

Hey *,

On 05.11. 09:08, Douglas C. Schmidt wrote:
>
> > Thanks! Will someone with commit access add the fixes directly or do
> > you need me to write a patch? Fopen is fine as well.
>
> Please grab the latest version from
>
> http://www.dre.vanderbilt.edu/~schmidt/DOC_ROOT/ACE/tests/Process_Test.cpp
>
> and send me the patches relative to that.

Here is the patch.

- Cleaned up the header files
- Removed ACE_OS::open() alltogether (Doug changed my original mktemp()
to mkstemp() which already takes
care of opening the tmp file)
- Removed an unneeded variable in main
- Changed the return value of run_parent() from int to void, since it's
not used anyway

Let's see what the daily build will say now.. ;)

Best,
Christian

---- BEGIN PATCH ----
--- /tmp/Process_Test.cpp 2008-11-05 16:36:33.427968239 +0100
+++ ./Process_Test.cpp 2008-11-05 16:39:37.106467528 +0100
@@ -21,10 +21,9 @@
#include "ace/Get_Opt.h"
#include "ace/ACE.h"
#include "ace/OS_NS_sys_stat.h"
+#include "ace/OS_NS_unistd.h"
#include "ace/Dirent.h"
-#include "ace/Vector_T.h"
#include "ace/SString.h"
-#include "ace/Process_Semaphore.h"

ACE_RCSID(tests, Process_Test, "Process_Test.cpp,v 4.11 1999/09/02 04:36:30 schmidt Exp")

@@ -79,7 +78,7 @@
return 0;
}

-int
+void
run_parent (bool inherit_files)
{
ACE_TCHAR t[] = "ace_testXXXXXX";
@@ -88,26 +87,15 @@
ACE_TCHAR tempfile[MAXPATHLEN + 1];

if (ACE::get_temp_dir (tempfile, MAXPATHLEN - sizeof (t)) == -1)
- ACE_ERROR_RETURN ((LM_ERROR,
- ACE_TEXT ("Could not get temp dir\n")),
- -1);
+ ACE_ERROR ((LM_ERROR,
+ ACE_TEXT ("Could not get temp dir\n")));

ACE_OS::strcat (tempfile, t);

int result = ACE_OS::mkstemp (tempfile);
if (result == -1)
- ACE_ERROR_RETURN ((LM_ERROR,
- ACE_TEXT ("Could not get temp filename\n")),
- -1);
-
- ACE_HANDLE file_handle = ACE_OS::open (tempfile,
- O_RDONLY|O_CREAT,
- ACE_DEFAULT_FILE_PERMS);
- if (file_handle == ACE_INVALID_HANDLE)
- ACE_ERROR_RETURN ((LM_ERROR,
- ACE_TEXT ("Could not open temp file: %s\n"),
- ACE_OS::strerror (ACE_OS::last_error ())),
- -1);
+ ACE_ERROR ((LM_ERROR,
+ ACE_TEXT ("Could not get temp filename\n")));

// Build child options
ACE_Process_Options options;
@@ -138,13 +126,12 @@
ACE_ERROR ((LM_ERROR,
ACE_TEXT ("Child %d finished with status %d\n"),
child.getpid (), child_status));
- return child_status;
}

int
run_main (int argc, ACE_TCHAR *argv[])
{
-#if defined (ACE_LACKS_FORK)
+#if defined (ACE_LACKS_FORK) || defined (ACE_LACKS_READLINK)
ACE_UNUSED_ARG (argc);
ACE_UNUSED_ARG (argv);

@@ -157,7 +144,6 @@
int c = 0;
int handle_inherit = 0; /* Disable inheritance by default */
bool ischild = false;
- ACE_Vector<ACE_CString> ofiles;
ACE_CString temp_file_name;

ACE_Get_Opt getopt (argc, argv, ACE_TEXT ("ch:f:"));
---- END PATCH ----

[Simon Massey]

I have applied the patch (slightly modified) to the repo and corrected
a few other problems. HOWEVER, I may be missing the point but dispite
the fact that a temporary file is created/opened, it is not used or
closed at all by this test. Shouldn't this file be at least closed by
the parent process and/or the child if it is inherited?

-----Original Message-----
From: Christian Fromme [mailto:chri...@ast.dfs.de]
Sent: Wednesday, 05 November 2008 15:45
To: Douglas C. Schmidt
Cc: Adam Mitz; buildczar; ace-...@list.isis.vanderbilt.edu
Subject: Re: [ace-users] [bczar] tests/Process_Test.cpp

[Christian Fromme]

Hi Simon,

On 06.11. 09:35, Simon Massey wrote:
> I have applied the patch (slightly modified) to the repo and corrected
> a few other problems. HOWEVER, I may be missing the point but dispite
> the fact that a temporary file is created/opened, it is not used or
> closed at all by this test. Shouldn't this file be at least closed by
> the parent process and/or the child if it is inherited?

Thanks for applying the patch. Please read the rest of this thread to
find out why this test was added. The file is of zero size, therefore it
shouldn't be a problem. Anyway, if you'd like to add an unlink() or
similar, feel free to do so.

Thanks,
Christian