apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@covalent.net>
Subject Re: file_setaside()
Date Mon, 18 Jun 2001 21:33:24 GMT

> I have a few questions about file_setaside.  I'm pasting the function in
> here for easy reference.
>
> --------------------------------------------------------------
> static apr_status_t file_setaside(apr_bucket *data, apr_pool_t *pool)
> {
>     apr_bucket_file *a = data->data;
>     apr_file_t *fd;
>     apr_file_t *f = a->fd;
>     apr_pool_t *p = apr_file_pool_get(f);
> #if APR_HAS_MMAP
>     apr_off_t filelength = data->length;  /* bytes remaining in file past
> offset
>  */
>     apr_off_t fileoffset = data->start;
> #endif
>
>     if (apr_pool_is_ancestor(p, pool)) {
>         return APR_SUCCESS;
>     }
>
> #if APR_HAS_MMAP
>     if (file_make_mmap(data, filelength, fileoffset, p)) {
>         return APR_SUCCESS;
>     }
> #endif
>     apr_file_dup(&fd, f, p);
>     a->fd = fd;
>     return APR_SUCCESS;
> }
> --------------------------------------------------------------
>
> (1) Shouldn't the pool passed into file_make_mmap() and apr_file_dup() be
> "pool" and not "p"?  (It'd be easier to see that this is a problem if they
> were called "reqpool" and "curpool" instead of "pool" and "p"
> respectively, or something like that.)

I think this is a bug, but I need to look closer......  Yep, bug.

> (2) Why should file_setaside mmap the file?  I'd think that we'd want to
> keep it as a file as long as possible to make it easier to use
> sendfile()... what am I missing?

We are going to be copying something.  I figured mmap'ing the file would
be a bit better, because we could write the file out.  Either way, it
doesn't really matter.

> (3) You don't really need to dup() the file, do you?  You can palloc a new
> apr_file_t in the requested pool and use apr_os_file_get() and
> apr_os_file_put() to move the os file handle into it.  mod_file_cache in
> Apache does something like this.  It should be cheaper to do this than to
> do a full dup(), I think.

The file was opened with the request->pool.  If we just
apr_os_file_get/put, we will still close the file when the request_pool is
cleared.  We have to dup, or the file won't be available to us, and the
original bug will be back.


Ryan


_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------


Mime
View raw message