apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: cvs commit: apr/threadproc/win32 proc.c
Date Wed, 03 Jul 2002 01:43:48 GMT
At 08:11 PM 7/2/2002, =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote:
>William A. Rowe, Jr. wrote:
>
>>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)

Yes, we should.  There are many places where console HANDLEs
are borked in APR right now.  I know that.  I also only have so much
time on my hands so I'm picking and choosing battles.

You are 100% right on - we should support odd HANDLE values (literally,
a console HANDLE generally has an odd value since it would never align,
and therefore signifies a non-NT object handle.)

But at the very least, apr_file_read on those handles is borked when the
buffer passed in is greater than some 30720 bytes.  Don't ask me, that
one isn't our bug, it's ms's borked tty API.

>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.

My patch isn't any more wrong than dozens of other places in the code
where we never had a return value, and later discover we need one.

I agree.  I committed the exact code to be changed in a separate commit,
as we introduce a return value for apr_foo_inherit_set/unset.  We've all
agreed that we will break the next tag (apache 2.0.40) in some somewhat
subtle ways regarding the return values, so let's just do it.

But it doesn't matter which order they are applied in.  It matters that these
are two seperate commits.

><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?

No.  I've broken it at least a dozen times in the past two years.
And it's been fixed reasonably promptly.  And so?

As far as a slightly more thorough test... well that wouldn't be testing
with Apache, would it?  It wouldn't be testing with the apr/test suite,
would it?  Care for me to be a mindreader?

For whatever reasons, the Win2K and XP boxes I'm hacking on lately
promoted the si.Handles to inherited all on their own.  Sorry that things
like that slip by.  Blame m$ for leniency towards incorrect API calls.

Next time commit additional test cases to apr/test when you are pissed
off, it helps us all solve these sorts of bugs and prevent them on the next
trip around that code.

><rant>
<counterrant>

>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?

I don't.  But you are developing against HEAD of how many different
code bases and projects?  I have at least a half dozen CVS head checkouts
here, several alpha and beta libraries.  Why?  So I can catch interactions
with the *next* set of bugs that will be released.

And I am usually bit in the middle of the night, or when I am in a rush,
or when I'd rather be doing something else.  Take the well intentioned
commit for apr_file_setaside().  It didn't work.  I've commented it out for
the moment so nobody trips.  But the tree will be broken sometimes.
cvs up -D"-24:00:00" and you will be much happier, and unstuck.

apr_proc_create stopped creating good processes for you.  revert the patch
in your own tree, and then YELL.  Don't just commit every hack that happens
to work out for you unless you are taking the time to figure out exactly what
is going on.  Obviously you know now.  Obviously you didn't when you reverted
the last two patches in one swoop.  Thank you for your willingness to fix the
broken code, I'm only asking that you would have done so before reverting
the good code.

>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?

I've been hacking on Apache for 2 years, and on APR for well over a year.
Hell, I wrote or rewrote a very large quantity of the Win32 code, with alot
of help from a very vocal peanut gallery.  And you know what?  Discussion
[when you disagree with a commit or the old code] accomplishes alot more.

The code is certainly not perfect [when held up as a stand alone library.]
Reverting patches without discussion is NOT forward progress to making
the entire library better.

>And you talk to me about bad form?

Yes.  You aren't the only one who's been clobbered by other folks' commits.
And if you want to get along as a Win32 committer, you need to deal with
a broken tree a good deal of the time in cross platform projects.

Learn to checkout by good, known date ... and commit incremental fixes
rather than reverting commits.

</counterrant>
></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.

Actually, remember there are others lurking here.  These sorts of discussions
about form or policies or whatever are productive, they help keep us all from
repeating the same mistakes.

So I apologize you tripped over a case that wasn't apparent on my development
OS's, that the existing API wasn't designed for.  And I apologize that code I
thought I had committed months ago to inherit.h wasn't there.  And I hope your
next 3am hacking session is alot more productive.

Bill


Mime
View raw message