You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Jonathan Gilbert <2j...@sneakemail.com> on 2007/02/19 07:30:00 UTC
[PATCH] Fix for Subversion issue #1789
Hello,
This is a repost of a message sent to the Subversion development mailing
list containing a patch for APR.
Windows has a bug in its WriteFile API. When the file handle is a console
output handle and the data chunk is greater than some value around 60 KB,
instead of doing either a complete write (which would be ideal) or a
partial write (which is permitted by the API contract), it returns an
incorrect error causing Subversion to display an error message such as:
svn: Can't write to stream: Not enough storage is available to process this
command.
Clearly, as the actual bug is in Windows, we can't fix the bug itself, so
we need a work-around, and common sense dictates that that work-around be
as close as possible to the API. So, the patch is for APR.
The obvious work-around is to take large writes and divide them up into
smaller writes. APR already uses separate files for file I/O on different
platforms, which means the work-around will be automatically segregated to
Win32 only. Even with the work-around in place, writing a large amount of
data in 48 KB blocks is not going to be significantly slower compared to
the ideal single API call. So, rather than add complexity and try to figure
out whether the file handle is a console file handle, I simply apply the
work-around to *all* file writes done through apr_file_write on Win32.
I've attached a patch which achieves this, however after an hour and a half
of trying to work with the Subversion build system for Windows (which I am
not familiar with), I was not able to get a working build. So, I present
this patch as "carefully reviewed & probably good but untested". I was
careful to preserve the semantics of the border case where *nbytes is
initially 0; as before, the function still calls WriteFile() with a count
of 0 once.
I would like to ask someone who does have a working Win32 build environment
using APR head to try applying my patch and see if it fixes the problem.
There is a test case for the bug at the following URL:
http://israel.logiclrd.cx/svn_bug.zip
Simply extract it to its own subdirectory and then issue "svn diff" within
that directory on a Windows platform. It should fail prior to the patch,
and, in theory, succeed with the patch applied.
If anyone would like to give me some hints on building under Windows, that
would also be great. I used "gen-make.py -t vcproj" as per the
instructions, but ended up with projects that do not reference their
dependencies and DLLs that do not export any functions at all.
Without further ado, here is the patch (inline & attached as I expect the
inline copy to be mangled by my e-mail client):
[[[
Fix issue #1789 by working around the underlying Windows bug.
* file_io/win32/readwrite.c: Made apr_file_write divide large buffers into
smaller chunks.
]]]
Index: file_io/win32/readwrite.c
===================================================================
--- file_io/win32/readwrite.c (revision 509010)
+++ file_io/win32/readwrite.c (working copy)
@@ -24,6 +24,16 @@
#include "apr_arch_atime.h"
#include "apr_arch_misc.h"
+/* This value is used to divide write operations into smaller
+ * chunks due to a bug in Windows that occurs when WriteFile
+ * is used on chunks larger than 64KB. Instead of doing a
+ * partial write (or internally looping in order to do a full
+ * write), it returns an error:
+ *
+ * "Not enough storage is available to process this command."
+ */
+#define MAXIMUM_SYNCHRONOUS_WRITE_SIZE (48 * 1024)
+
/*
* read_with_timeout()
* Uses async i/o to emulate unix non-blocking i/o with timeouts.
@@ -309,9 +319,48 @@
if (thefile->pOverlapped) {
thefile->pOverlapped->Offset = (DWORD)thefile->filePtr;
thefile->pOverlapped->OffsetHigh =
(DWORD)(thefile->filePtr >> 32);
+ rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes,
+ &bwrote, thefile->pOverlapped);
}
- rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
- thefile->pOverlapped);
+ else {
+ /* The write may need to be divided up into chunks due to a
+ * bug in WriteFile when it is given a console file handle.
+ */
+ bwrote = 0;
+ do {
+ /* Determine the next chunk's size and write it. */
+ DWORD chunk_size = MAXIMUM_SYNCHRONOUS_WRITE_SIZE;
+
+ if (chunk_size > *nbytes) {
+ chunk_size = *nbytes;
+ }
+
+ rv = WriteFile(thefile->filehand, buf,
+ chunk_size, &chunk_size, NULL);
+
+ /* If an error occurs but some bytes were written, report
+ * success for those bytes only. A subsequent call will
+ * return the same error without any bytes having been
+ * written.
+ */
+ if (!rv) {
+ if (bwrote > 0) {
+ /* By making rv non-zero, the return-value test
+ * below will return APR_SUCCESS.
+ */
+ rv = TRUE;
+ }
+ break;
+ }
+
+ /* Tally the bytes that were written and advance the
+ * buffer pointer. */
+ bwrote += chunk_size;
+
+ buf = chunk_size + (char *)buf;
+ *nbytes -= chunk_size;
+ } while (*nbytes > 0);
+ }
if (thefile->append) {
apr_file_unlock(thefile);
apr_thread_mutex_unlock(thefile->mutex);
Re: [PATCH] Fix for Subversion issue #1789
Posted by Jonathan Gilbert <2j...@sneakemail.com>.
At 03:16 AM 2/19/2007 -0600, William A. Rowe, Jr. wrote:
>Jonathan Gilbert wrote:
>>
>> This is a repost of a message sent to the Subversion development mailing
>> list containing a patch for APR.
>
>Thanks for forwarding. In the future, you might want to also log this sort
>of patch to issues.apache.org/bugzilla/ - to catch the maintainer's attention
>the PatchAvailable bug keyword marks it as very-low-hanging fruit.
Okay. :-)
>Question, are you the author? If not, could that author please submit us
>their patch directly so we follow our IP constraints? Otherwise this
>discussion is off the table till I have time to have someone else look at
>this patch who hasn't followed the emails.
I am the author of the patch. :-)
>> Windows has a bug in its WriteFile API. When the file handle is a console
>> output handle and the data chunk is greater than some value around 60 KB,
>> instead of doing either a complete write (which would be ideal) or a
>> partial write (which is permitted by the API contract), it returns an
>> incorrect error causing Subversion to display an error message such as:
>
>I've heard 32000 at one time /shrug. It might vary by os release.
That is entirely possible :-) I did do an extremely simple test case on
Windows XP Service Pack 2 and found that a single 70 KB block could not be
written without the patch, but went through fine as a 48 KB block followed
by a 22 KB block with the patch applied, which would support the ~60 KB the
original bug reporter gave in the Subversion Issue Tracker, but that's the
only OS I have to test on.
>Forget it :) I believe you underestimate things on a major level; consider
>someone copying an ISO image. VERY measurable in terms of kernel state
>transitions, depending on what the destination driver/device is.
Consider that Windows itself, when copying a file from one place to another
in, say, Exploder uses 64 KB blocks for that copy operation. Are you
suggesting that Exploder's file copies are significantly slower than they
would be if they used, say, a 2 MB block for the copy operation? CMD.EXE
uses the same block size, and other utilities often use smaller blocks.
I could do a benchmark, though I don't have any particularly high-end
hardware to try this on, but my experience in the past has been that the
overhead becomes close to insignificant anywhere over 8 KB.
In any event, I think your proposal below makes sense :-)
>> I've attached a patch which achieves this, however after an hour and a half
>> of trying to work with the Subversion build system for Windows (which I am
>> not familiar with), I was not able to get a working build. So, I present
>> this patch as "carefully reviewed & probably good but untested". I was
>> careful to preserve the semantics of the border case where *nbytes is
>> initially 0; as before, the function still calls WriteFile() with a count
>> of 0 once.
>
>Thanks for the heads up :)
For what it's worth, as mentioned above, I did do a minimal unit test of
the APR function itself; I just wasn't able to try it through Subversion
and see if I had actually patched the place where console output from the
command-line utility eventually reached.
>> If anyone would like to give me some hints on building under Windows, that
>> would also be great. I used "gen-make.py -t vcproj" as per the
>> instructions, but ended up with projects that do not reference their
>> dependencies and DLLs that do not export any functions at all.
>
>No clue at all, FWIW if all you are building is apr, there is a visual
>studio .dsw (workspace) that modern studios will convert to the .sln
>solution file on open. The -win32-src.zip sources include a makefile
>generated from the .dsw/.dsp files. (.sln/.vcproj files won't export
>to .mak files, so we've refused to migrate so far.)
>
>As far as gen-make.py - I don't personally use it so can't vouch for it.
Yeah, actually I had no problems at all building APR :-) It worked perfectly.
Someone on the Subversion list suggested that perhaps Visual Studio needs
to be able to find the Python runtime in order to perform the build, so I'm
going to try that at some point and see if it fixes the problem -- with any
luck, it will, and then I'll be able to say whether my patch cured
Subversion's symptoms.
>> + * "Not enough storage is available to process this command."
>> + */
>> +#define MAXIMUM_SYNCHRONOUS_WRITE_SIZE (48 * 1024)
>
>Safer to set this to 32000 per earlier console pipe documention.
>
>I'd veto the solution below, as I noted above you make a really horrid
>assumption that writing small chunks would be efficient for writing
>out tens or hundreds of megabytes to disk. But we have a small,
>saving grace...
I don't think 48 KB chunks aren't really particularly small, but..
>"Can't write to stream: Not enough storage is available to process this
>command."
>
>What if we caught that exception after the existing writefile statement,
>and in the specific case of that error, we were to use your patch below
>to shuttle in one 32000 segment at a time? It would significantly slow
>those writes for drivers or pipes subject to this limit, BUT I believe
>that's the rare exception (most writes being within the size) and we
>benefit from not slowing down 'just for consoles'.
>
>What are your thoughts on that approach?
It sounds sensible, though it will require more structural change to the
Win32 apr_file_write function than the current patch. I'll take a look at
it tonight.
Jonathan Gilbert
Re: [PATCH] Fix for Subversion issue #1789
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Feb 19, 2007 at 10:50:36AM -0600, Jonathan Gilbert wrote:
> At 09:33 AM 2/19/2007 +0000, Joe Orton wrote:
> >On Mon, Feb 19, 2007 at 12:30:00AM -0600, Jonathan Gilbert wrote:
> >> The obvious work-around is to take large writes and divide them up into
> >> smaller writes
> >
> >When not just constrain the length to 32K (doing so iff the file is a
> >console output handle, if that's cheap to handle).
> >
> > if (*nbytes > MAX_VALUE) *nbytes = MAX_VALUE;
> >
> >and just do a partial write; as you say, the API allows this.
>
> This is the smallest possible change to achieve the basic functionality
> described, but it has two drawbacks:
Any caller using apr_file_write() and not checking for partial writes is
broken and will break in cases less obscure than this,
apr_file_write_full() has been in APR forever.
joe
Re: [PATCH] Fix for Subversion issue #1789
Posted by Jonathan Gilbert <2j...@sneakemail.com>.
God, I need to get more sleep. This is a great first impression I'm making :-)
At 10:50 AM 2/19/2007 -0600, I wrote:
[snip]
>minimize the effect on performance and behaviour, especially for existing
>callers that aren't triggering this bug and doesn't even know that a change
>is needed.
[snip]
"that don't even know"
At 10:39 AM 2/19/2007 -0600, I wrote:
[snip]
>Consider that Windows itself, when copying a file from one place to another
>in, say, Exploder uses 64 KB blocks for that copy operation.
[snip]
Not a typo ;-) A comma is missing, though.
>I don't think 48 KB chunks aren't really particularly small, but..
s/aren't/are/
I'm normally very careful about my use of language. I know you probably
don't care, but I do :-)
Jonathan Gilbert
Re: [PATCH] Fix for Subversion issue #1789
Posted by Jonathan Gilbert <2j...@sneakemail.com>.
At 09:33 AM 2/19/2007 +0000, Joe Orton wrote:
>On Mon, Feb 19, 2007 at 12:30:00AM -0600, Jonathan Gilbert wrote:
>> The obvious work-around is to take large writes and divide them up into
>> smaller writes
>
>When not just constrain the length to 32K (doing so iff the file is a
>console output handle, if that's cheap to handle).
>
> if (*nbytes > MAX_VALUE) *nbytes = MAX_VALUE;
>
>and just do a partial write; as you say, the API allows this.
This is the smallest possible change to achieve the basic functionality
described, but it has two drawbacks:
1. It places the onus on checking for partial writes, especially where they
are not expected, on the caller. While all callers should absolutely be
doing this without exception, many people aren't careful and are spoilt by
prior good experience with filesystem write buffering and read combining
and such. Even when they are careful, correctly advancing through a buffer,
while trivial, is an opportunity for human error. Getting it right in one
place is a fundamental goal of having a library like APR.
2. Even when the caller does correctly handle partial writes coming back
from the APR function, it makes the path the code must follow between
successive hits to the API much longer and more convoluted. In many cases,
this *will* affect performance, especially when called from
script/script-like languages like Perl.
While it is a worthy goal to minimize the effect on the source code, I
think for a project with APR's profile, it is a more important goal to
minimize the effect on performance and behaviour, especially for existing
callers that aren't triggering this bug and doesn't even know that a change
is needed. William Rowe's suggestion to only divide the buffer into smaller
chunks if trying to write the whole buffer fails with the error message
known to be caused by the API bug actually increases the code complexity a
little bit over my initial patch, but there is the potential for a
significant reduction in the impact the patch has on situations the bug
doesn't affect.
Jonathan Gilbert
Re: [PATCH] Fix for Subversion issue #1789
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
William A. Rowe, Jr. wrote:
> Jonathan Gilbert wrote:
>> This is a repost of a message sent to the Subversion development mailing
>> list containing a patch for APR.
>
> Thanks for forwarding. In the future, you might want to also log this sort
> of patch to issues.apache.org/bugzilla/ - to catch the maintainer's attention
> the PatchAvailable bug keyword marks it as very-low-hanging fruit.
>
> Question, are you the author? If not, could that author please submit us
> their patch directly so we follow our IP constraints? Otherwise this
> discussion is off the table till I have time to have someone else look at
> this patch who hasn't followed the emails.
s/patch/bug/ of course. The point is that it needs to be fixed, but clean
room patching is a PITA that some ones (plural) need to make time for.
Looking forward to good news one way or the other.
Re: [PATCH] Fix for Subversion issue #1789
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Jonathan Gilbert wrote:
>
> This is a repost of a message sent to the Subversion development mailing
> list containing a patch for APR.
Thanks for forwarding. In the future, you might want to also log this sort
of patch to issues.apache.org/bugzilla/ - to catch the maintainer's attention
the PatchAvailable bug keyword marks it as very-low-hanging fruit.
Question, are you the author? If not, could that author please submit us
their patch directly so we follow our IP constraints? Otherwise this
discussion is off the table till I have time to have someone else look at
this patch who hasn't followed the emails.
> Windows has a bug in its WriteFile API. When the file handle is a console
> output handle and the data chunk is greater than some value around 60 KB,
> instead of doing either a complete write (which would be ideal) or a
> partial write (which is permitted by the API contract), it returns an
> incorrect error causing Subversion to display an error message such as:
I've heard 32000 at one time /shrug. It might vary by os release.
> svn: Can't write to stream: Not enough storage is available to process this
> command.
Hmmm - so we have a reproduceable error result. Worth noting...
> Clearly, as the actual bug is in Windows, we can't fix the bug itself, so
> we need a work-around, and common sense dictates that that work-around be
> as close as possible to the API. So, the patch is for APR.
Agreed.
> The obvious work-around is to take large writes and divide them up into
> smaller writes. APR already uses separate files for file I/O on different
> platforms, which means the work-around will be automatically segregated to
> Win32 only. Even with the work-around in place, writing a large amount of
> data in 48 KB blocks is not going to be significantly slower compared to
> the ideal single API call. So, rather than add complexity and try to figure
> out whether the file handle is a console file handle, I simply apply the
> work-around to *all* file writes done through apr_file_write on Win32.
Forget it :) I believe you underestimate things on a major level; consider
someone copying an ISO image. VERY measurable in terms of kernel state
transitions, depending on what the destination driver/device is.
> I've attached a patch which achieves this, however after an hour and a half
> of trying to work with the Subversion build system for Windows (which I am
> not familiar with), I was not able to get a working build. So, I present
> this patch as "carefully reviewed & probably good but untested". I was
> careful to preserve the semantics of the border case where *nbytes is
> initially 0; as before, the function still calls WriteFile() with a count
> of 0 once.
Thanks for the heads up :)
> If anyone would like to give me some hints on building under Windows, that
> would also be great. I used "gen-make.py -t vcproj" as per the
> instructions, but ended up with projects that do not reference their
> dependencies and DLLs that do not export any functions at all.
No clue at all, FWIW if all you are building is apr, there is a visual
studio .dsw (workspace) that modern studios will convert to the .sln
solution file on open. The -win32-src.zip sources include a makefile
generated from the .dsw/.dsp files. (.sln/.vcproj files won't export
to .mak files, so we've refused to migrate so far.)
As far as gen-make.py - I don't personally use it so can't vouch for it.
> + * "Not enough storage is available to process this command."
> + */
> +#define MAXIMUM_SYNCHRONOUS_WRITE_SIZE (48 * 1024)
Safer to set this to 32000 per earlier console pipe documention.
I'd veto the solution below, as I noted above you make a really horrid
assumption that writing small chunks would be efficient for writing
out tens or hundreds of megabytes to disk. But we have a small,
saving grace...
"Can't write to stream: Not enough storage is available to process this
command."
What if we caught that exception after the existing writefile statement,
and in the specific case of that error, we were to use your patch below
to shuttle in one 32000 segment at a time? It would significantly slow
those writes for drivers or pipes subject to this limit, BUT I believe
that's the rare exception (most writes being within the size) and we
benefit from not slowing down 'just for consoles'.
What are your thoughts on that approach?
> @@ -309,9 +319,48 @@
> if (thefile->pOverlapped) {
> thefile->pOverlapped->Offset = (DWORD)thefile->filePtr;
> thefile->pOverlapped->OffsetHigh =
> (DWORD)(thefile->filePtr >> 32);
> + rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes,
> + &bwrote, thefile->pOverlapped);
> }
> - rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
> - thefile->pOverlapped);
> + else {
> + /* The write may need to be divided up into chunks due to a
> + * bug in WriteFile when it is given a console file handle.
> + */
> + bwrote = 0;
> + do {
> + /* Determine the next chunk's size and write it. */
> + DWORD chunk_size = MAXIMUM_SYNCHRONOUS_WRITE_SIZE;
> +
> + if (chunk_size > *nbytes) {
> + chunk_size = *nbytes;
> + }
> +
> + rv = WriteFile(thefile->filehand, buf,
> + chunk_size, &chunk_size, NULL);
> +
> + /* If an error occurs but some bytes were written, report
> + * success for those bytes only. A subsequent call will
> + * return the same error without any bytes having been
> + * written.
> + */
> + if (!rv) {
> + if (bwrote > 0) {
> + /* By making rv non-zero, the return-value test
> + * below will return APR_SUCCESS.
> + */
> + rv = TRUE;
> + }
> + break;
> + }
> +
> + /* Tally the bytes that were written and advance the
> + * buffer pointer. */
> + bwrote += chunk_size;
> +
> + buf = chunk_size + (char *)buf;
> + *nbytes -= chunk_size;
> + } while (*nbytes > 0);
> + }
> if (thefile->append) {
> apr_file_unlock(thefile);
> apr_thread_mutex_unlock(thefile->mutex);
Re: [PATCH] Fix for Subversion issue #1789
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Feb 19, 2007 at 12:30:00AM -0600, Jonathan Gilbert wrote:
> Windows has a bug in its WriteFile API. When the file handle is a console
> output handle and the data chunk is greater than some value around 60 KB,
> instead of doing either a complete write (which would be ideal) or a
> partial write (which is permitted by the API contract), it returns an
> incorrect error causing Subversion to display an error message such as:
>
> svn: Can't write to stream: Not enough storage is available to process this
> command.
>
> Clearly, as the actual bug is in Windows, we can't fix the bug itself, so
> we need a work-around, and common sense dictates that that work-around be
> as close as possible to the API. So, the patch is for APR.
>
> The obvious work-around is to take large writes and divide them up into
> smaller writes
When not just constrain the length to 32K (doing so iff the file is a
console output handle, if that's cheap to handle).
if (*nbytes > MAX_VALUE) *nbytes = MAX_VALUE;
and just do a partial write; as you say, the API allows this.
joe
Re: [PATCH] Fix for Subversion issue #1789
Posted by Jonathan Gilbert <2j...@sneakemail.com>.
At 11:36 AM 2/19/2007 -0600, William A. Rowe, Jr. wrote:
>Jonathan Gilbert wrote:
>> Consider that Windows itself, when copying a file from one place to another
>> in, say, Exploder uses 64 KB blocks for that copy operation. Are you
>> suggesting that Exploder's file copies are significantly slower than they
>> would be if they used, say, a 2 MB block for the copy operation? CMD.EXE
>> uses the same block size, and other utilities often use smaller blocks.
>
>Uhm, try xcopy /s/v/e src dst ... then compare exploder ^c / ^v... orders
>of magnitude slower (for a host of reasons, I'm certain). Want optimal?
>mmap source, write source to dest in a single operation :)
XCOPY is likely just more efficient at scanning directory subtrees. I
tested it; it also uses a 64KB buffer size. There may be something else it
does, but it certainly isn't getting massive speed from its buffer size.
>> Someone on the Subversion list suggested that perhaps Visual Studio needs
>> to be able to find the Python runtime in order to perform the build, [snip]
>
>That's quite possibly true of some .py wrapper stuff, although we've
>completely eliminated the need for awk / perl / python in our nmake of
>libapr / libaprutil.
Yeah. Well, APR built just fine for me right off the bat. I haven't tried
the Subversion fix yet, but I will shortly.
>>> What are your thoughts on that approach?
>>
>> It sounds sensible, though it will require more structural change to the
>> Win32 apr_file_write function than the current patch. I'll take a look at
>> it tonight.
>
>One thought; once we identify that it will fails, we could write a segment
>some 32000 bytes, mark it for 'small io', and return (as Joe suggests).
>
>One beauty of apr_file_t (we wish it was true for all) - it's opaque; we
>can modify the struct to have a small_file_ops flag or something like that,
>and avert the error-case in the future by forcing one 32k write.
Ah, I hadn't realized apr_file_t was an opaque platform-specific thing that
could be modified almost at a whim. I suppose it would have occurred to me
if I'd thought about it much :-)
>A larger patch, I know, especially with initializing that flag value to
>zero in all the right places.
Actually, the patch contains significantly less new code. I discovered that
APR already implements buffered file I/O and, with the current
implementation, enforces the buffer size even on large writes through
apr_file_write. It's perfect for chopping the write up into smaller chunks :-)
The default buffer size is currently only 4 KB, and there was no way to
override it that I saw, so I had two choices: Use the existing buffer
initialization or implement buffer allocation separately for console
handles. Doing the former places an upper limit on the value of
APR_DEFAULT_FILE_BUFSIZE, since of course we can't depend on WriteFile
being able to send more than 32,000 bytes at once to a console handle, so I
opted for the second option, with its own macro APR_CONSOLE_BUFSIZE in
include/arch/win32/apr_arch_file_io.h. As noted in the comment block, I
chose a buffer size of 28,672 bytes because it is the largest multiple of
the x86 system page size below 32,000 bytes.
I also factored out the common console file handle initialization into a
new method open_console_handle() which is called by the three
apr_file_open_flags_* functions. The new method includes code to initialize
the console handle's output buffer using APR_CONSOLE_BUFSIZE.
The only catch with buffering is that by default, it will actually buffer
data and nothing will appear until an entire buffer-full is queued up. To
get around this, I added a platform-specific apr_file_open flag called
APR_AUTOFLUSH_BUFFER. When the flag is present, apr_file_flush() is called
after each apr_file_write() finishes chewing through the buffer. This
ensures that small writes, including the partial buffer at the end of a
large write, do get sent to the console.
I think this is a lot more elegant than the previous version of the patch
:-) Please find the patch itself inline and attached.
Jonathan Gilbert
Index: include/arch/win32/apr_arch_file_io.h
===================================================================
--- include/arch/win32/apr_arch_file_io.h (revision 509010)
+++ include/arch/win32/apr_arch_file_io.h (working copy)
@@ -86,6 +86,14 @@
/* For backwards-compat */
#define APR_FILE_BUFSIZE APR_FILE_DEFAULT_BUFSIZE
+/* The WriteFile API fails when used on a console handle if the data
+ * buffer is too large. On Windows XP SP2, the boundary seems to be
+ * around 60 KB, but on earlier versions of Windows, it may be around
+ * 32,000 bytes. This buffer size is the largest multiple of the Win32
+ * page size less than 32,000.
+ */
+#define APR_CONSOLE_BUFSIZE 28672
+
/* obscure ommissions from msvc's sys/stat.h */
#ifdef _MSC_VER
#define S_IFIFO _S_IFIFO /* pipe */
@@ -96,11 +104,12 @@
#endif
/* Internal Flags for apr_file_open */
-#define APR_OPENINFO 0x00100000 /* Open without READ or WRITE access */
-#define APR_OPENLINK 0x00200000 /* Open a link itself, if supported */
-#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_OPENINFO 0x00100000 /* Open without READ or WRITE
access */
+#define APR_OPENLINK 0x00200000 /* Open a link itself, if
supported */
+#define APR_READCONTROL 0x00400000 /* Read the file's owner/perms */
+#define APR_WRITECONTROL 0x00800000 /* Modify the file's owner/perms */
+#define APR_WRITEATTRS 0x01000000 /* Modify the file's attributes */
+#define APR_AUTOFLUSH_BUFFER 0x02000000 /* Flush the buffer after every
write */
/* Entries missing from the MSVC 5.0 Win32 SDK:
*/
Index: file_io/win32/open.c
===================================================================
--- file_io/win32/open.c (revision 509010)
+++ file_io/win32/open.c (working copy)
@@ -534,7 +534,7 @@
(*file)->bufsize = APR_FILE_DEFAULT_BUFSIZE;
}
- if ((*file)->append || (*file)->buffered) {
+ if ((*file)->append || (*file)->buffered || (flags &
APR_AUTOFLUSH_BUFFER)) {
apr_status_t rv;
rv = apr_thread_mutex_create(&(*file)->mutex,
APR_THREAD_MUTEX_DEFAULT, pool);
@@ -567,17 +567,19 @@
return APR_SUCCESS;
}
-APR_DECLARE(apr_status_t) apr_file_open_flags_stderr(apr_file_t **thefile,
- apr_int32_t flags,
- apr_pool_t *pool)
+apr_status_t open_console_handle(apr_file_t **thefile,
+ DWORD handle_id,
+ apr_int32_t flags,
+ apr_pool_t *pool)
{
#ifdef _WIN32_WCE
return APR_ENOTIMPL;
#else
apr_os_file_t file_handle;
+ apr_status_t rv;
apr_set_os_error(APR_SUCCESS);
- file_handle = GetStdHandle(STD_ERROR_HANDLE);
+ file_handle = GetStdHandle(handle_id);
if (!file_handle || (file_handle == INVALID_HANDLE_VALUE)) {
apr_status_t rv = apr_get_os_error();
if (rv == APR_SUCCESS) {
@@ -586,54 +588,56 @@
return rv;
}
- return apr_os_file_put(thefile, &file_handle, flags | APR_WRITE, pool);
+ /* Set the APR_AUTOFLUSH_BUFFER flag. The existing
+ * buffer functionality is used to handle large
+ * writes, but we don't want small writes (including
+ * the remainder that will typically be present from
+ * a large write) to sit around in the buffer. The
+ * APR_AUTOFLUSH_BUFFER flag triggers a call to
+ * apr_file_flush() at the end of each apr_file_write().
+ */
+ flags |= APR_AUTOFLUSH_BUFFER;
+
+ rv = apr_os_file_put(thefile, &file_handle, flags, pool);
+
+ /* If we're successful and the handle is to an output
+ * stream, set up a write buffer to ensure that large
+ * console write operations don't fail. The WriteFile
+ * API can't handle operations larger than a certain
+ * size. This is done separately here instead of
+ * setting flag APR_BUFFERED because the buffer size
+ * is different from the APR_FILE_DEFAULT_BUFSIZE of
+ * 4KB.
+ */
+ if ((rv == APR_SUCCESS) && (flags & APR_WRITE)) {
+ (*thefile)->buffered = 1;
+ (*thefile)->buffer = apr_palloc(pool, APR_CONSOLE_BUFSIZE);
+ (*thefile)->bufsize = APR_CONSOLE_BUFSIZE;
+ }
+
+ return rv;
#endif
}
+APR_DECLARE(apr_status_t) apr_file_open_flags_stderr(apr_file_t **thefile,
+ apr_int32_t flags,
+ apr_pool_t *pool)
+{
+ return open_console_handle(thefile, STD_ERROR_HANDLE, flags |
APR_WRITE, pool);
+}
+
APR_DECLARE(apr_status_t) apr_file_open_flags_stdout(apr_file_t **thefile,
apr_int32_t flags,
apr_pool_t *pool)
{
-#ifdef _WIN32_WCE
- return APR_ENOTIMPL;
-#else
- apr_os_file_t file_handle;
-
- 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;
- }
-
- return apr_os_file_put(thefile, &file_handle, flags | APR_WRITE, pool);
-#endif
+ return open_console_handle(thefile, STD_OUTPUT_HANDLE, flags |
APR_WRITE, pool);
}
APR_DECLARE(apr_status_t) apr_file_open_flags_stdin(apr_file_t **thefile,
apr_int32_t flags,
apr_pool_t *pool)
{
-#ifdef _WIN32_WCE
- return APR_ENOTIMPL;
-#else
- apr_os_file_t file_handle;
-
- 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;
- }
-
- return apr_os_file_put(thefile, &file_handle, flags | APR_READ, pool);
-#endif
+ return open_console_handle(thefile, STD_INPUT_HANDLE, flags |
APR_READ, pool);
}
APR_DECLARE(apr_status_t) apr_file_open_stderr(apr_file_t **thefile,
apr_pool_t *pool)
Index: file_io/win32/readwrite.c
===================================================================
--- file_io/win32/readwrite.c (revision 509010)
+++ file_io/win32/readwrite.c (working copy)
@@ -282,6 +282,19 @@
size -= blocksize;
}
+ if ((rv == APR_SUCCESS) && (thefile->flags & APR_AUTOFLUSH_BUFFER)) {
+ /* The file handle is marked with the APR_AUTOFLUSH_BUFFER
+ * flag. This is primarily used for Win32 console output
+ * handles, for which the WriteFile API function can't
+ * handle single writes above a certain size. We use the
+ * existing buffer functionality because it chops the data
+ * into bite-sized pieces nicely, but we don't actually
+ * want the handle to be fully buffered, waiting for a
+ * full buffer before displaying a single line of text.
+ */
+ rv = apr_file_flush(thefile);
+ }
+
apr_thread_mutex_unlock(thefile->mutex);
return rv;
} else {