William A. Rowe, Jr. wrote:
At 06:55 PM 7/2/2002, Brane wrote:
William A. Rowe, Jr. wrote:
If there is a problem, it is NOT in this patch you reverted. It is probably
localized to apr_file_inherit_set(). That API didn't exist when the original
'make inheritable duplicates' was added.
The first order if business is to get HEAD working again, regardless.
The code wasn't correct in the first place. Ergo reverting to a pseudo-working
state isn't productive.
The difference is between "working" and "not working", not "buggy" and "not buggy". I know why you made the change, and we all agree that the original was buggy, and I'm in the process of testing a better patch (O.K., you beat me to it, but anyway).
Neither is reverting and then reapplying a patch.
The correct fix was 10 minutes. A heads up to the list is ALWAYS warranted
unless vetoed code is checked into the repository.
Um. Sorry to disagree here, but the fix isn't entirely correct. You can't use SetHandleInformation for console buffers on pre-Win2k variants, including WinNT 4.0 and below. Do we care about that? Probably not for Apache, but we should for other reasons (not the least that I suspect Subversion won't work on NT4 now)
And your patch is obviously wrong because it won't detect a failure to make the handle inheritable. I know that's because the declarations of those functions are wrong, but making them correct is definitely more than 10 minutes' work. My patch avoids that problem by (again, temporarily) avoiding those APis.
I'm attaching my patch, just for comparison.
We do NOT have to roll over each other every time that HEAD is broken.
If it doesn't compile, fine. But you are talking about a behavioral problem
with the apr_foo_set_inherit semantics, so you rip out a perfectly legit patch.
That's bad form.
<rant>
Are you aware this is the second time you broke apr_proc_create and friends, when a slightly more thorough test would have avoided it? And why do you think HEAD should remain non-working (but non-buggy?) while I figure out that we have a no-op funciton that's causing problems? That it's 3 a.m. and I threw away several hours of work trying to find a bug in Subversion that wasn't there? And you talk to me about bad form?
</rant>
I'll probably regret this last paragraph tomorrow, but I'm too pissed off to regret it now. Apologies to innocent bystanders. We should take this off list now, mano y mano.
-- Brane Äibej <[EMAIL PROTECTED]> http://www.xbc.nu/brane/
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/win32/proc.c,v
retrieving revision 1.79
diff -u -r1.79 proc.c
--- proc.c 3 Jul 2002 00:23:39 -0000 1.79
+++ proc.c 3 Jul 2002 01:02:39 -0000
@@ -129,7 +129,13 @@
return APR_SUCCESS;
}
-static apr_status_t make_handle_private(apr_file_t *file)
+/* XXX We should be using apr_file_inherit_set and
+ * apr_file_inherit_unset instead of make_handle_private and
+ * make_handle_inheritable. However, those functions are declared
+ * void; the declaration has to be changed to apr_status_t before we
+ * can move this implementation into file_io/win32/open.c. */
+
+static apr_status_t set_handle_inheritance(apr_file_t *file, BOOL how)
{
#ifdef _WIN32_WCE
return APR_ENOTIMPL;
@@ -144,7 +150,7 @@
*/
if (!DuplicateHandle(hproc, filehand,
hproc, &file->filehand, 0,
- FALSE, DUPLICATE_SAME_ACCESS))
+ how, DUPLICATE_SAME_ACCESS))
return apr_get_os_error();
/*
* Close the inerhitable handle we don't need anymore.
@@ -154,6 +160,17 @@
#endif
}
+static apr_status_t make_handle_private(apr_file_t *file)
+{
+ return set_handle_inheritance(file, FALSE);
+}
+
+static apr_status_t make_handle_inheritable(apr_file_t *file)
+{
+ return set_handle_inheritance(file, TRUE);
+}
+
+
APR_DECLARE(apr_status_t) apr_procattr_io_set(apr_procattr_t *attr,
apr_int32_t in,
apr_int32_t out,
@@ -195,7 +212,7 @@
rv = apr_file_dup2(attr->child_in, child_in, attr->pool);
if (rv == APR_SUCCESS)
- apr_file_inherit_set(attr->child_in);
+ rv = make_handle_inheritable(attr->child_in);
}
if (parent_in && rv == APR_SUCCESS) {
@@ -221,7 +238,7 @@
rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
if (rv == APR_SUCCESS)
- apr_file_inherit_set(attr->child_out);
+ rv = make_handle_inheritable(attr->child_out);
}
if (parent_out && rv == APR_SUCCESS) {
@@ -247,7 +264,7 @@
rv = apr_file_dup2(attr->child_err, child_err, attr->pool);
if (rv == APR_SUCCESS)
- apr_file_inherit_set(attr->child_err);
+ rv = make_handle_inheritable(attr->child_err);
}
if (parent_err && rv == APR_SUCCESS) {
