You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by wr...@apache.org on 2007/10/01 22:22:04 UTC
svn commit: r581063 - in /apr/apr/branches/1.2.x: CHANGES
file_io/win32/filedup.c file_io/win32/open.c include/apr_thread_proc.h
include/arch/win32/apr_arch_file_io.h threadproc/win32/proc.c
Author: wrowe
Date: Mon Oct 1 13:22:04 2007
New Revision: 581063
URL: http://svn.apache.org/viewvc?rev=581063&view=rev
Log:
Introduce APR_NO_FILE as an option to apr_procattr_io_set() for any
of the three stdio streams to cause the corresponding streams to NOT
be inherited to a WIN32 child process (passes INVALID_HANDLE_VALUE).
This is of little importance to most developers, except those who had
exploited an inconsistency between Unix and Win32 that was corrected
on Win32 with APR version 1.2.10.
The feature to have NO_FILE work across platforms and leave a specified
std stream closed becomes portable in APR version 1.3.0 (it remains
equivilant to APR_NO_PIPE on non-Windows platforms throughout version
APR 1.2.x).
Backports: r580484
No backport to later patches for other platforms.
Modified:
apr/apr/branches/1.2.x/CHANGES
apr/apr/branches/1.2.x/file_io/win32/filedup.c
apr/apr/branches/1.2.x/file_io/win32/open.c
apr/apr/branches/1.2.x/include/apr_thread_proc.h
apr/apr/branches/1.2.x/include/arch/win32/apr_arch_file_io.h
apr/apr/branches/1.2.x/threadproc/win32/proc.c
Modified: apr/apr/branches/1.2.x/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/CHANGES?rev=581063&r1=581062&r2=581063&view=diff
==============================================================================
--- apr/apr/branches/1.2.x/CHANGES [utf-8] (original)
+++ apr/apr/branches/1.2.x/CHANGES [utf-8] Mon Oct 1 13:22:04 2007
@@ -1,7 +1,16 @@
- -*- coding: utf-8 -*-
+ -*- coding: utf-8 -*-
Changes for APR 1.2.12
-
+ *) Introduce APR_NO_FILE as an option to apr_procattr_io_set() for any
+ of the three stdio streams to cause the corresponding streams to NOT
+ be inherited to a WIN32 child process (passes INVALID_HANDLE_VALUE).
+ This is of little importance to most developers, except those who had
+ exploited an inconsistency between Unix and Win32 that was corrected
+ on Win32 with APR version 1.2.10. The feature to have NO_FILE work
+ across platforms and leave a specified std stream closed becomes
+ portable in APR version 1.3.0 (it remains equivilant to APR_NO_PIPE
+ on non-Windows platforms throughout version APR 1.2.x).
+ [William Rowe]
Changes for APR 1.2.11
Modified: apr/apr/branches/1.2.x/file_io/win32/filedup.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/file_io/win32/filedup.c?rev=581063&r1=581062&r2=581063&view=diff
==============================================================================
--- apr/apr/branches/1.2.x/file_io/win32/filedup.c (original)
+++ apr/apr/branches/1.2.x/file_io/win32/filedup.c Mon Oct 1 13:22:04 2007
@@ -20,6 +20,8 @@
#include "apr_strings.h"
#include <string.h>
#include "apr_arch_inherit.h"
+#include <io.h> /* for [_open/_get]_osfhandle */
+
APR_DECLARE(apr_status_t) apr_file_dup(apr_file_t **new_file,
apr_file_t *old_file, apr_pool_t *p)
@@ -63,10 +65,6 @@
#endif /* !defined(_WIN32_WCE) */
}
-#define stdin_handle 0x01
-#define stdout_handle 0x02
-#define stderr_handle 0x04
-
APR_DECLARE(apr_status_t) apr_file_dup2(apr_file_t *new_file,
apr_file_t *old_file, apr_pool_t *p)
{
@@ -77,32 +75,88 @@
HANDLE hproc = GetCurrentProcess();
HANDLE newhand = NULL;
apr_int32_t newflags;
+ int fd;
- /* dup2 is not supported literaly with native Windows handles.
- * We can, however, emulate dup2 for the standard i/o handles,
- * and close and replace other handles with duped handles.
- * The os_handle will change, however.
- */
- if (new_file->filehand == GetStdHandle(STD_ERROR_HANDLE)) {
- stdhandle |= stderr_handle;
- }
- if (new_file->filehand == GetStdHandle(STD_OUTPUT_HANDLE)) {
- stdhandle |= stdout_handle;
- }
- if (new_file->filehand == GetStdHandle(STD_INPUT_HANDLE)) {
- stdhandle |= stdin_handle;
- }
+ if (new_file->flags & APR_STD_FLAGS) {
+ /* if (!DuplicateHandle(hproc, old_file->filehand,
+ * hproc, &newhand, 0,
+ * TRUE, DUPLICATE_SAME_ACCESS)) {
+ * return apr_get_os_error();
+ * }
+ * if (((stdhandle & stderr_handle) && !SetStdHandle(STD_ERROR_HANDLE, newhand)) ||
+ * ((stdhandle & stdout_handle) && !SetStdHandle(STD_OUTPUT_HANDLE, newhand)) ||
+ * ((stdhandle & stdin_handle) && !SetStdHandle(STD_INPUT_HANDLE, newhand))) {
+ * return apr_get_os_error();
+ * }
+ * newflags = old_file->flags | APR_INHERIT;
+ */
+
+ if ((new_file->flags & APR_STD_FLAGS) == APR_STDERR_FLAG) {
+ /* Flush stderr and unset its buffer, then commit the fd-based buffer.
+ * This is typically a noop for Win2K/XP since services with NULL std
+ * handles [but valid FILE *'s, oddly enough], but is required
+ * for NT 4.0 and to use this code outside of services.
+ */
+ fflush(stderr);
+ setvbuf(stderr, NULL, _IONBF, 0);
+ _commit(2 /* stderr */);
+
+ /* Clone a handle can _close() without harming the source handle,
+ * open an MSVCRT-based pseudo-fd for the file handle, then dup2
+ * and close our temporary pseudo-fd once it's been duplicated.
+ * This will incidently keep the FILE-based stderr in sync.
+ * Note the apparently redundant _O_BINARY coersions are required.
+ */
+ if (!DuplicateHandle(hproc, old_file->filehand, hproc, &newhand,
+ 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+ return apr_get_os_error();
+ }
+ fd = _open_osfhandle((INT_PTR)newhand, _O_WRONLY | _O_BINARY);
+ _dup2(fd, 2);
+ _close(fd);
+ _setmode(2, _O_BINARY);
+
+ /* hPipeWrite was _close()'ed above, and _dup2()'ed
+ * to fd 2 creating a new, inherited Win32 handle.
+ * Recover that real handle from fd 2. Note that
+ * SetStdHandle(STD_ERROR_HANDLE, _get_osfhandle(2))
+ * is implicit in the dup2() call above
+ */
+ newhand = (HANDLE)_get_osfhandle(2);
+ }
- if (stdhandle) {
- if (!DuplicateHandle(hproc, old_file->filehand,
- hproc, &newhand, 0,
- TRUE, DUPLICATE_SAME_ACCESS)) {
- return apr_get_os_error();
+ if ((new_file->flags & APR_STD_FLAGS) == APR_STDOUT_FLAG) {
+ /* For the process flow see the stderr case above */
+ fflush(stdout);
+ setvbuf(stdout, NULL, _IONBF, 0);
+ _commit(1 /* stdout */);
+
+ if (!DuplicateHandle(hproc, old_file->filehand, hproc, &newhand,
+ 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+ return apr_get_os_error();
+ }
+ fd = _open_osfhandle((INT_PTR)newhand, _O_WRONLY | _O_BINARY);
+ _dup2(fd, 1);
+ _close(fd);
+ _setmode(1, _O_BINARY);
+ newhand = (HANDLE)_get_osfhandle(1);
}
- if (((stdhandle & stderr_handle) && !SetStdHandle(STD_ERROR_HANDLE, newhand)) ||
- ((stdhandle & stdout_handle) && !SetStdHandle(STD_OUTPUT_HANDLE, newhand)) ||
- ((stdhandle & stdin_handle) && !SetStdHandle(STD_INPUT_HANDLE, newhand))) {
- return apr_get_os_error();
+
+ if ((new_file->flags & APR_STD_FLAGS) == APR_STDIN_FLAG) {
+ /* For the process flow see the stderr case above */
+ fflush(stdin);
+ setvbuf(stdin, NULL, _IONBF, 0);
+ _commit(0 /* stdin */);
+
+ if (!DuplicateHandle(hproc, old_file->filehand, hproc, &newhand,
+ 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+ return apr_get_os_error();
+ }
+ fd = _open_osfhandle((INT_PTR)newhand, _O_RDONLY | _O_BINARY);
+ _dup2(fd, 0);
+ _close(fd);
+ _setmode(0, _O_BINARY);
+ newhand = (HANDLE)_get_osfhandle(0);
}
newflags = old_file->flags | APR_INHERIT;
}
Modified: apr/apr/branches/1.2.x/file_io/win32/open.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/file_io/win32/open.c?rev=581063&r1=581062&r2=581063&view=diff
==============================================================================
--- apr/apr/branches/1.2.x/file_io/win32/open.c (original)
+++ apr/apr/branches/1.2.x/file_io/win32/open.c Mon Oct 1 13:22:04 2007
@@ -574,15 +574,11 @@
apr_set_os_error(APR_SUCCESS);
file_handle = GetStdHandle(STD_ERROR_HANDLE);
- if (!file_handle || (file_handle == INVALID_HANDLE_VALUE)) {
- apr_status_t rv = apr_get_os_error();
- if (rv == APR_SUCCESS) {
- return APR_EINVAL;
- }
- return rv;
- }
+ if (!file_handle)
+ file_handle = INVALID_HANDLE_VALUE;
- return apr_os_file_put(thefile, &file_handle, 0, pool);
+ return apr_os_file_put(thefile, &file_handle,
+ APR_WRITE | APR_STDERR_FLAG, pool);
#endif
}
@@ -595,15 +591,11 @@
apr_set_os_error(APR_SUCCESS);
file_handle = GetStdHandle(STD_OUTPUT_HANDLE);
- if (!file_handle || (file_handle == INVALID_HANDLE_VALUE)) {
- apr_status_t rv = apr_get_os_error();
- if (rv == APR_SUCCESS) {
- return APR_EINVAL;
- }
- return rv;
- }
+ if (!file_handle)
+ file_handle = INVALID_HANDLE_VALUE;
- return apr_os_file_put(thefile, &file_handle, 0, pool);
+ return apr_os_file_put(thefile, &file_handle,
+ APR_WRITE | APR_STDOUT_FLAG, pool);
#endif
}
@@ -616,15 +608,11 @@
apr_set_os_error(APR_SUCCESS);
file_handle = GetStdHandle(STD_INPUT_HANDLE);
- if (!file_handle || (file_handle == INVALID_HANDLE_VALUE)) {
- apr_status_t rv = apr_get_os_error();
- if (rv == APR_SUCCESS) {
- return APR_EINVAL;
- }
- return rv;
- }
+ if (!file_handle)
+ file_handle = INVALID_HANDLE_VALUE;
- return apr_os_file_put(thefile, &file_handle, 0, pool);
+ return apr_os_file_put(thefile, &file_handle,
+ APR_READ | APR_STDIN_FLAG, pool);
#endif
}
Modified: apr/apr/branches/1.2.x/include/apr_thread_proc.h
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/include/apr_thread_proc.h?rev=581063&r1=581062&r2=581063&view=diff
==============================================================================
--- apr/apr/branches/1.2.x/include/apr_thread_proc.h (original)
+++ apr/apr/branches/1.2.x/include/apr_thread_proc.h Mon Oct 1 13:22:04 2007
@@ -87,6 +87,15 @@
/** @see apr_procattr_io_set */
#define APR_CHILD_BLOCK 4
+/** @see apr_procattr_io_set
+ * @note introduced strictly for Win32 to apr revision 1.2.12 (to restore
+ * the non-portable default behavior of 1.2.9 and prior versions on Win32).
+ * This becomes portable to all platforms effective revision 1.3.0, ensuring
+ * the standard files specified in the call to apr_procattr_io_set are not
+ * open in the created process (on Win32 as INVALID_HANDLE_VALUEs).
+ */
+#define APR_NO_FILE 8
+
/** @see apr_procattr_limit_set */
#define APR_LIMIT_CPU 0
/** @see apr_procattr_limit_set */
Modified: apr/apr/branches/1.2.x/include/arch/win32/apr_arch_file_io.h
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/include/arch/win32/apr_arch_file_io.h?rev=581063&r1=581062&r2=581063&view=diff
==============================================================================
--- apr/apr/branches/1.2.x/include/arch/win32/apr_arch_file_io.h (original)
+++ apr/apr/branches/1.2.x/include/arch/win32/apr_arch_file_io.h Mon Oct 1 13:22:04 2007
@@ -99,6 +99,10 @@
#define APR_READCONTROL 0x00400000 /* Read the file's owner/perms */
#define APR_WRITECONTROL 0x00800000 /* Modifythe file's owner/perms */
#define APR_WRITEATTRS 0x01000000 /* Modify the file's attributes */
+#define APR_STDIN_FLAG 0x02000000 /* Obtained via apr_file_open_stdin() */
+#define APR_STDOUT_FLAG 0x04000000 /* Obtained via apr_file_open_stdout() */
+#define APR_STDERR_FLAG 0x06000000 /* Obtained via apr_file_open_stderr() */
+#define APR_STD_FLAGS (APR_STDIN_FLAG | APR_STDOUT_FLAG | APR_STDERR_FLAG)
/* Entries missing from the MSVC 5.0 Win32 SDK:
*/
Modified: apr/apr/branches/1.2.x/threadproc/win32/proc.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/threadproc/win32/proc.c?rev=581063&r1=581062&r2=581063&view=diff
==============================================================================
--- apr/apr/branches/1.2.x/threadproc/win32/proc.c (original)
+++ apr/apr/branches/1.2.x/threadproc/win32/proc.c Mon Oct 1 13:22:04 2007
@@ -32,6 +32,15 @@
#include <process.h>
#endif
+/* Heavy on no'ops, here's what we want to pass if there is APR_NO_FILE
+ * requested for a specific child handle;
+ */
+static apr_file_t no_file = { NULL, INVALID_HANDLE_VALUE, };
+
+/* We have very carefully excluded volumes of definitions from the
+ * Microsoft Platform SDK, which kill the build time performance.
+ * These the sole constants we borrow from WinBase.h and WinUser.h
+ */
#ifndef LOGON32_LOGON_NETWORK
#define LOGON32_LOGON_NETWORK 3
#endif
@@ -83,20 +92,30 @@
in = APR_READ_BLOCK;
else if (in == APR_PARENT_BLOCK)
in = APR_WRITE_BLOCK;
- stat = apr_create_nt_pipe(&attr->child_in, &attr->parent_in, in,
- attr->pool);
+
+ if (in == APR_NO_FILE)
+ attr->child_in = &no_file;
+ else
+ stat = apr_create_nt_pipe(&attr->child_in, &attr->parent_in,
+ in, attr->pool);
if (stat == APR_SUCCESS)
stat = apr_file_inherit_unset(attr->parent_in);
}
if (out && stat == APR_SUCCESS) {
- stat = apr_create_nt_pipe(&attr->parent_out, &attr->child_out, out,
- attr->pool);
+ if (out == APR_NO_FILE)
+ attr->child_out = &no_file;
+ else
+ stat = apr_create_nt_pipe(&attr->parent_out, &attr->child_out,
+ out, attr->pool);
if (stat == APR_SUCCESS)
stat = apr_file_inherit_unset(attr->parent_out);
}
if (err && stat == APR_SUCCESS) {
- stat = apr_create_nt_pipe(&attr->parent_err, &attr->child_err, err,
- attr->pool);
+ if (err == APR_NO_FILE)
+ attr->child_err = &no_file;
+ else
+ stat = apr_create_nt_pipe(&attr->parent_err, &attr->child_err,
+ err, attr->pool);
if (stat == APR_SUCCESS)
stat = apr_file_inherit_unset(attr->parent_err);
}
@@ -110,7 +129,7 @@
apr_status_t rv = APR_SUCCESS;
if (child_in) {
- if (attr->child_in == NULL)
+ if ((attr->child_in == NULL) || (attr->child_in == &no_file))
rv = apr_file_dup(&attr->child_in, child_in, attr->pool);
else
rv = apr_file_dup2(attr->child_in, child_in, attr->pool);
@@ -136,7 +155,7 @@
apr_status_t rv = APR_SUCCESS;
if (child_out) {
- if (attr->child_out == NULL)
+ if ((attr->child_out == NULL) || (attr->child_out == &no_file))
rv = apr_file_dup(&attr->child_out, child_out, attr->pool);
else
rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
@@ -162,7 +181,7 @@
apr_status_t rv = APR_SUCCESS;
if (child_err) {
- if (attr->child_err == NULL)
+ if ((attr->child_err == NULL) || (attr->child_err == &no_file))
rv = apr_file_dup(&attr->child_err, child_err, attr->pool);
else
rv = apr_file_dup2(attr->child_err, child_err, attr->pool);
@@ -784,9 +803,10 @@
SetHandleInformation(si.hStdInput,
HANDLE_FLAG_INHERIT, 0);
- si.hStdInput = attr->child_in->filehand;
- SetHandleInformation(si.hStdInput, HANDLE_FLAG_INHERIT,
- HANDLE_FLAG_INHERIT);
+ if ( (si.hStdInput = attr->child_in->filehand)
+ != INVALID_HANDLE_VALUE )
+ SetHandleInformation(si.hStdInput, HANDLE_FLAG_INHERIT,
+ HANDLE_FLAG_INHERIT);
}
si.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
@@ -798,9 +818,10 @@
SetHandleInformation(si.hStdOutput,
HANDLE_FLAG_INHERIT, 0);
- si.hStdOutput = attr->child_out->filehand;
- SetHandleInformation(si.hStdOutput, HANDLE_FLAG_INHERIT,
- HANDLE_FLAG_INHERIT);
+ if ( (si.hStdOutput = attr->child_out->filehand)
+ != INVALID_HANDLE_VALUE )
+ SetHandleInformation(si.hStdOutput, HANDLE_FLAG_INHERIT,
+ HANDLE_FLAG_INHERIT);
}
si.hStdError = GetStdHandle(STD_ERROR_HANDLE);
@@ -812,9 +833,10 @@
SetHandleInformation(si.hStdError,
HANDLE_FLAG_INHERIT, 0);
- si.hStdError = attr->child_err->filehand;
- SetHandleInformation(si.hStdError, HANDLE_FLAG_INHERIT,
- HANDLE_FLAG_INHERIT);
+ if ( (si.hStdError = attr->child_err->filehand)
+ != INVALID_HANDLE_VALUE )
+ SetHandleInformation(si.hStdError, HANDLE_FLAG_INHERIT,
+ HANDLE_FLAG_INHERIT);
}
}
if (attr->user_token) {
@@ -937,13 +959,13 @@
new->hproc = pi.hProcess;
new->pid = pi.dwProcessId;
- if (attr->child_in) {
+ if ((attr->child_in) && (attr->child_in != &no_file)) {
apr_file_close(attr->child_in);
}
- if (attr->child_out) {
+ if ((attr->child_out) && (attr->child_out != &no_file)) {
apr_file_close(attr->child_out);
}
- if (attr->child_err) {
+ if ((attr->child_err) && (attr->child_err != &no_file)) {
apr_file_close(attr->child_err);
}
CloseHandle(pi.hThread);
Re: svn commit: r581063 - in /apr/apr/branches/1.2.x: CHANGES file_io/win32/filedup.c file_io/win32/open.c include/apr_thread_proc.h include/arch/win32/apr_arch_file_io.h threadproc/win32/proc.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Oct 02, 2007 at 01:54:03PM -0500, William Rowe wrote:
> Joe Orton wrote:
> >
> > This is an API extension so can't be backported to the 1.2.x branch (per
> > the versioning rules).
>
> Which explains why you responded to the original post how?
>
> In a rare turn, this doesn't actually introduce any API. I was very explicit
> in pointing this out, and asking a week ago for the feedback of the developers.
I'm not sure how adding a new macro to a public header and adding a new
interface guarantee if that macro is used can be considered as anything
other than "introducing a new API".
If the procattr API is so ill-defined that it can't be implemented
portably and it requires interface extensions to fix that, so be it (no
argument here), but the versioning rules require that such changes have
to wait for 1.3.0.
joe
Re: svn commit: r581063 - in /apr/apr/branches/1.2.x: CHANGES file_io/win32/filedup.c
file_io/win32/open.c include/apr_thread_proc.h include/arch/win32/apr_arch_file_io.h
threadproc/win32/proc.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>
> This is an API extension so can't be backported to the 1.2.x branch (per
> the versioning rules).
Which explains why you responded to the original post how?
In a rare turn, this doesn't actually introduce any API. I was very explicit
in pointing this out, and asking a week ago for the feedback of the developers.
Let's be clear;
1.2.8 was not portable behavior; violated APR, but was the API for Win32.
1.2.11 restored portability to the Unix model; broke a few, far between
applications of APR on Win32.
1.2.12 proposes to retain the restored portability in all but the most
unusual edge cases who presumed our API for Win32 was correct.
I'd be happy to keep this, remove the APR_NO_FILE flag, hint to that
application developer of the hardcoded constant, and let them roll their
dice for expecting APR to provide grok that constant. With APR 1.3/httpd 2.4
they would be able to get rid of the const value cruft.
If you want to scroll back and respond to the final analysis thread named;
"Solution to apr stdio/msvc crt/service handles and logging"
... I took the time to thoroughly dissect the issue, if you want to weight
in, that would be the thread to do it on.
We have two purposes to serve on the 1.2.x branch; let people expect portable
behaviors, and attempt to avoid breaking people who weren't looking at the
portability of behaviors.
This is the only compromise I could arrive at. It doesn't require changes
to functions, or arg types, or return types. It's strictly the introduction
of a MACRO which will not 'harm' any existing app.
If you want to veto the patch; propose an alternative that serves both the
purpose of portability and the purpose of those who relied on the older non
portable behavior.
Bill
Re: svn commit: r581063 - in /apr/apr/branches/1.2.x: CHANGES file_io/win32/filedup.c file_io/win32/open.c include/apr_thread_proc.h include/arch/win32/apr_arch_file_io.h threadproc/win32/proc.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Oct 01, 2007 at 08:22:04PM -0000, William Rowe wrote:
> Author: wrowe
> Date: Mon Oct 1 13:22:04 2007
> New Revision: 581063
>
> URL: http://svn.apache.org/viewvc?rev=581063&view=rev
> Log:
> Introduce APR_NO_FILE as an option to apr_procattr_io_set() for any
> of the three stdio streams to cause the corresponding streams to NOT
> be inherited to a WIN32 child process (passes INVALID_HANDLE_VALUE).
>
> This is of little importance to most developers, except those who had
> exploited an inconsistency between Unix and Win32 that was corrected
> on Win32 with APR version 1.2.10.
>
> The feature to have NO_FILE work across platforms and leave a specified
> std stream closed becomes portable in APR version 1.3.0 (it remains
> equivilant to APR_NO_PIPE on non-Windows platforms throughout version
> APR 1.2.x).
This is an API extension so can't be backported to the 1.2.x branch (per
the versioning rules).
joe