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
