brian at brianfrance
Apr 4, 2012, 5:11 PM
Post #3 of 3
(110 views)
Permalink
|
|
Re: mod_dav_fs does not check for return value on stream_close
[In reply to]
|
|
Yes, you are correct. Looks like my merge from work code to httpd svn didn't fully work.
We have been running this patch for a week or so with no issues.
Brian
On Apr 4, 2012, at 6:24 PM, Graham Leggett wrote:
> On 15 Mar 2012, at 3:56 PM, Brian J. France wrote:
>
>> Could somebody review the patch below for 2.2, 2.4, and trunk?
>>
>> A better error message could be sent, but I am more worried about how the return will effect the code after it.
>>
>> I am thinking the file needs to be removed either via a apr_file_remove call or:
>>
>> apr_pool_cleanup_kill(stream->p, stream, tmpfile_cleanup);
>>
>> call, but I don't know the code well enough to know which is right and 2.4/trunk adds even more complexity compared to 2.2.x.
>>
>> Thoughts/Comments?
>>
>> Thanks,
>>
>> Brian
>>
>> - I am still getting more details why close is failing, but for some reason it is happening enough to cause issues. (responding 200, but no file)
>>
>> -----
>>
>> 2.2:
>>
>> Index: modules/dav/fs/repos.c
>> ===================================================================
>> --- modules/dav/fs/repos.c (revision 1300964)
>> +++ modules/dav/fs/repos.c (working copy)
>> @@ -881,6 +881,10 @@
>> {
>> apr_file_close(stream->f);
>
> Would be above not be
>
> status = apr_file_close(stream->f);
>
>>
>> + if (status != APR_SUCCESS) {
>> + return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
>> + }
>> +
>> if (!commit) {
>> if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
>> /* ### use a better description? */
>>
>>
>> 2.24 and trunk:
>>
>>
>> Index: modules/dav/fs/repos.c
>> ===================================================================
>> --- modules/dav/fs/repos.c (revision 1300964)
>> +++ modules/dav/fs/repos.c (working copy)
>> @@ -970,6 +970,10 @@
>>
>> apr_file_close(stream->f);
>
> Same with this one.
>
>>
>> + if (status != APR_SUCCESS) {
>> + return dav_new_error(stream->p, MAP_IO2HTTP(status), 0, "There was a problem closing the stream");
>> + }
>> +
>> if (!commit) {
>> if (stream->temppath) {
>> apr_pool_cleanup_run(stream->p, stream, tmpfile_cleanup);
>>
>>
>
> Regards,
> Graham
> --
>
|
smime.p7s
