apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jonathan Gilbert" <2jx64e...@sneakemail.com>
Subject Re: [PATCH] Fix for Subversion issue #1789
Date Mon, 19 Feb 2007 16:50:36 GMT
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

Mime
View raw message