Re: Inherited Handles and APR
On Tue, 17 Jul 2001, William A. Rowe, Jr. wrote: We agree. APR_INHERIT is the 'option', APR_NON_INHERIT is a zero (usual case) value. my complaint is that you're modifying all the handle creation APIs with another parameter... -dean
Re: Inherited Handles and APR
On Tue, Jul 17, 2001 at 08:32:47PM -0500, William A. Rowe, Jr. wrote: Then, I'm +1 on _not_ passing this as an argument to the creation fn, rather assuming that the inherit flag is _not_ set, and force the user to call apr_socket_inherit_set or apr_file_inherit_set. Does this work for _everyone_? +1. Much nicer. -- justin
Re: Inherited Handles and APR
+1 from me too (although i don't know that i necessarily have a vote in APR :) -dean On Tue, 17 Jul 2001, Justin Erenkrantz wrote: On Tue, Jul 17, 2001 at 08:32:47PM -0500, William A. Rowe, Jr. wrote: Then, I'm +1 on _not_ passing this as an argument to the creation fn, rather assuming that the inherit flag is _not_ set, and force the user to call apr_socket_inherit_set or apr_file_inherit_set. Does this work for _everyone_? +1. Much nicer. -- justin
Re: Inherited Handles and APR
Anyone care to apply this to unix and hack it a bit? I only found four cases
where
we (probably) should be calling apr_file_open() to create an inheritable file.
As I mention, sockets are still an issue, so are child processes, shared memory
handles, and a list of others. But if we knock these off one bit at a time, we
will
probably knock down the child handle abuse.
Serious (?) bug I noticed, if a buffered file has pending data to write, and a
child
is spawned, both the parent _AND_ child will attempt to flush the buffer. It
used
to happen when the child pool was destroyed, with this patch it will occur when
the
process is spawned. It appears we need a pre-fork() registered action, to flush
buffers and the like. That's only partially effective though, unless we do some
heavy mutex protection :(
I'm still traveling till Sunday morning, so I can't revisit for a bit.
- Original Message -
From: William A. Rowe, Jr. [EMAIL PROTECTED]
To: dev@apr.apache.org
Sent: Friday, July 13, 2001 3:45 PM
Subject: Inherited Handles and APR
After a respectable lunch at Boudin's, Ryan and I think we have the general
answers
to child handles.
The apr_foo_open/create calls need an APR_INHERIT flag bit to mark resources
as
inheritable. This offers two advantages;
1. the app doesn't need to worry about fork/exec cleanups v.s. inherit bits
for
createprocess() based Win32 (and other non-forks.)
2. mainline code is more readable, as the user no longer registers their own
cleanup_for_exec handlers to simply close handles.
The patch, for apr_file_open, follows. Rbb is working on a similar for
Sockets, and
calls that generate 'handles' will need to be reviewed as well...
I obviously have the much larger job of applying the APR_INHERIT flag
judiciously
throughout the entire server and removing child cleanups where appropriate ...
Bill
Index: file_io/unix/open.c
===
RCS file: /home/cvs/apr/file_io/unix/open.c,v
retrieving revision 1.77
diff -u -r1.77 open.c
--- file_io/unix/open.c 2001/06/27 19:40:34 1.77
+++ file_io/unix/open.c 2001/07/13 20:41:08
@@ -167,7 +167,8 @@
(*new)-dataRead = 0;
(*new)-direction = 0;
apr_pool_cleanup_register((*new)-cntxt, (void *)(*new),
apr_unix_file_cleanup,
-apr_pool_cleanup_null);
+ (flag APR_INHERIT) ? apr_pool_cleanup_null
+ : apr_unix_file_cleanup);
return APR_SUCCESS;
}
Index: file_io/win32/open.c
===
RCS file: /home/cvs/apr/file_io/win32/open.c,v
retrieving revision 1.77
diff -u -r1.77 open.c
--- file_io/win32/open.c2001/06/27 19:40:41 1.77
+++ file_io/win32/open.c2001/07/13 20:41:10
@@ -173,6 +173,7 @@
apr_int32_t flag, apr_fileperms_t perm,
apr_pool_t *cont)
{
+SECURITY_ATTRIBUTES sa, *psa = NULL;
/* XXX: The default FILE_FLAG_SEQUENTIAL_SCAN is _wrong_ for
* sdbm and any other random files! We _must_ rethink
* this approach.
@@ -236,6 +237,12 @@
*/
attributes |= FILE_FLAG_OVERLAPPED;
}
+if (flag APR_INHERIT) {
+sa.nLength = sizeof(sa);
+sa.bInheritHandle = TRUE;
+sa.lpSecurityDescriptor = NULL;
+psa = sa;
+}
#if APR_HAS_UNICODE_FS
if (os_level = APR_WIN_NT) {
@@ -244,12 +251,12 @@
/ sizeof(apr_wchar_t), fname))
return rv;
handle = CreateFileW(wfname, oflags, sharemode,
- NULL, createflags, attributes, 0);
+ psa, createflags, attributes, 0);
}
else
#endif
handle = CreateFileA(fname, oflags, sharemode,
- NULL, createflags, attributes, 0);
+ psa, createflags, attributes, 0);
if (handle == INVALID_HANDLE_VALUE) {
return apr_get_os_error();
Index: include/apr_file_io.h
===
RCS file: /home/cvs/apr/include/apr_file_io.h,v
retrieving revision 1.103
diff -u -r1.103 apr_file_io.h
--- include/apr_file_io.h 2001/06/27 19:40:46 1.103
+++ include/apr_file_io.h 2001/07/13 20:41:13
@@ -89,6 +89,10 @@
#define APR_SHARELOCK 1024/* Platform dependent support for higher
level locked read/write access to support
writes across process/machines */
+#define APR_INHERIT4096/* Create the file inheritable by the child
+ process. fork()ed implementations
+ automatically register a child cleanup
Inherited Handles and APR
After a respectable lunch at Boudin's, Ryan and I think we have the general
answers
to child handles.
The apr_foo_open/create calls need an APR_INHERIT flag bit to mark resources as
inheritable. This offers two advantages;
1. the app doesn't need to worry about fork/exec cleanups v.s. inherit bits
for
createprocess() based Win32 (and other non-forks.)
2. mainline code is more readable, as the user no longer registers their own
cleanup_for_exec handlers to simply close handles.
The patch, for apr_file_open, follows. Rbb is working on a similar for
Sockets, and
calls that generate 'handles' will need to be reviewed as well...
I obviously have the much larger job of applying the APR_INHERIT flag
judiciously
throughout the entire server and removing child cleanups where appropriate ...
Bill
Index: file_io/unix/open.c
===
RCS file: /home/cvs/apr/file_io/unix/open.c,v
retrieving revision 1.77
diff -u -r1.77 open.c
--- file_io/unix/open.c 2001/06/27 19:40:34 1.77
+++ file_io/unix/open.c 2001/07/13 20:41:08
@@ -167,7 +167,8 @@
(*new)-dataRead = 0;
(*new)-direction = 0;
apr_pool_cleanup_register((*new)-cntxt, (void *)(*new),
apr_unix_file_cleanup,
-apr_pool_cleanup_null);
+ (flag APR_INHERIT) ? apr_pool_cleanup_null
+ : apr_unix_file_cleanup);
return APR_SUCCESS;
}
Index: file_io/win32/open.c
===
RCS file: /home/cvs/apr/file_io/win32/open.c,v
retrieving revision 1.77
diff -u -r1.77 open.c
--- file_io/win32/open.c2001/06/27 19:40:41 1.77
+++ file_io/win32/open.c2001/07/13 20:41:10
@@ -173,6 +173,7 @@
apr_int32_t flag, apr_fileperms_t perm,
apr_pool_t *cont)
{
+SECURITY_ATTRIBUTES sa, *psa = NULL;
/* XXX: The default FILE_FLAG_SEQUENTIAL_SCAN is _wrong_ for
* sdbm and any other random files! We _must_ rethink
* this approach.
@@ -236,6 +237,12 @@
*/
attributes |= FILE_FLAG_OVERLAPPED;
}
+if (flag APR_INHERIT) {
+sa.nLength = sizeof(sa);
+sa.bInheritHandle = TRUE;
+sa.lpSecurityDescriptor = NULL;
+psa = sa;
+}
#if APR_HAS_UNICODE_FS
if (os_level = APR_WIN_NT) {
@@ -244,12 +251,12 @@
/ sizeof(apr_wchar_t), fname))
return rv;
handle = CreateFileW(wfname, oflags, sharemode,
- NULL, createflags, attributes, 0);
+ psa, createflags, attributes, 0);
}
else
#endif
handle = CreateFileA(fname, oflags, sharemode,
- NULL, createflags, attributes, 0);
+ psa, createflags, attributes, 0);
if (handle == INVALID_HANDLE_VALUE) {
return apr_get_os_error();
Index: include/apr_file_io.h
===
RCS file: /home/cvs/apr/include/apr_file_io.h,v
retrieving revision 1.103
diff -u -r1.103 apr_file_io.h
--- include/apr_file_io.h 2001/06/27 19:40:46 1.103
+++ include/apr_file_io.h 2001/07/13 20:41:13
@@ -89,6 +89,10 @@
#define APR_SHARELOCK 1024/* Platform dependent support for higher
level locked read/write access to support
writes across process/machines */
+#define APR_INHERIT4096/* Create the file inheritable by the child
+ process. fork()ed implementations
+ automatically register a child cleanup
+ in the _absence_ of this flag. */
/* flags for apr_file_seek */
#define APR_SET SEEK_SET
@@ -141,6 +145,10 @@
* APR_SHARELOCKPlatform dependent support for higher
*level locked read/write access to support
*writes across process/machines
+ * APR_INHERIT Create the file inheritable by the child
+ *processes. fork()ed implementations
+ *automatically register a child cleanup
+ *in the _absence_ of this flag.
* /PRE
* @param perm Access permissions for file.
* @param cont The pool to use.
