|
Jim Gallacher
jpg at jgassociates.ca
Tue Jun 6 13:20:38 EDT 2006
Graham Dumpleton wrote: > Sergey A. Lipnevich wrote .. >> Hi All, >> >> I'd like to offer the following patch for 3.2.8's sendfile function that >> asks APR to provide less information about a file (which is beneficial >> on Windows as APR doesn't have to construct POSIX protection flags), >> because the only field in use becomes file size. Incidentally, it also >> allows mod_python (plus Apache 2.2 patch) to work with the following >> tweak for APR 1.2.7 (required for Subversion), under Windows and Apache >> 2.2.2: http://article.gmane.org/gmane.comp.apache.apr.devel/9389/. >> >> I didn't test the patch on anything but Windows XP. The error I was >> getting with the setup above was "Could not stat file for reading." >> I also have to mention that the whole thing was compiled with VS 2005 >> compiler to support Python 2.4, although I think it's not relevant for >> the patch attached. >> >> I hope all this patch citing is not too confusing :-). > > Part of this patch may break UNIX systems. Specifically, when target is a > symbolic link, the size returned will be the length of the filename and > not the size of the target file when SIZE and not NORM is used. Thus > an incorrect amount of data is sent back. I applied this patch on my Linux development machine (apache 2.0.55) last week, and have not seen any problems - but then I've not really used sendfile in the interim, so why would I? ;) Mention of the symlink issue twigged a memory of a prior problem, and sure enough APR_FINFO_NORM was introduced to fix this very issue. See http://issues.apache.org/jira/browse/MODPYTHON-84. The patch for that issue looks like this: --- httpd/mod_python/trunk/src/requestobject.c 2005/10/15 15:27:11 321348 +++ httpd/mod_python/trunk/src/requestobject.c 2005/10/15 15:31:46 321349 @@ -1024,7 +1024,7 @@ Py_BEGIN_ALLOW_THREADS status=apr_stat(&finfo, fname, - APR_READ, self->request_rec->pool); + APR_FINFO_NORM, self->request_rec->pool); Py_END_ALLOW_THREADS if (status != APR_SUCCESS) { PyErr_SetString(PyExc_IOError, "Could not stat file for reading"); I don't recall dissecting that bug as it was a one line fix. Looking back at that issue, the previous code was incorrectly using APR_READ instead of one of the APR_FINFO_* macros. As it happens APR_READ has the same value as APR_FINFO_LINK, (which stats the link not the file itself, if it is link), thus resulting the reported bug. We only need to stat the file to get the file size. APR_FINFO_NORM does this, but as Sergey points out it drags in alot of unwanted stuff as well. As I read it APR_FINFO_SIZE will do what we want, and his patch does not introduce a regression. The unit test seems to confirm this. If this analysis seems correct I'll reopen MODPYTHON-84 and commit Sergey's patch against that issue. (I don't see the point in opening a new issue). I think this fix would be a good candidate for a backport to the 3.2.x branch for inclusion in 3.2.9. Jim
|