You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Lars Eilebrecht <la...@eilebrecht.net> on 2009/02/07 21:02:57 UTC
use of APR_SENDFILE_ENABLED in mod_disk_cache
Hi,
mod_disk_cache uses the following in open_entity():
#ifdef APR_SENDFILE_ENABLED
flags |= APR_SENDFILE_ENABLED;
#endif
rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
Maybe I'm getting confused with the various APR_SENDFILE
defines, but shouldn't we be checking for the setting of
the EnableSendFile directive as well?
If httpd was build on a platform providing sendfile support
then I guess APR_SENDFILE_ENABLED is set and we would always
be using sendfile when serving files from the cache,
regardless of the setting of the EnableSendFile directive.
ciao...
--
Lars Eilebrecht
lars@eilebrecht.net
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Lars Eilebrecht <la...@eilebrecht.net>.
Issac Goldstand wrote:
> > We could just add a note to the mod_disk_cache configuration that
> > EnableSendfile will only be taken into account when configured
> > globally for the server or vhost. IMHO that's good enough for
> > such a special case. I would like to avoid a dedicated sendfile
> > directive just for mod_disk_cache.
> >
> > ciao...
>
> +1
Oh, looks like you reported the same bug like 2 years ago, but
it wasn't fixed in trunk as you mentioned in the PR.
ciao...
--
Lars Eilebrecht
lars@eilebrecht.net
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Issac Goldstand <ma...@beamartyr.net>.
Lars Eilebrecht wrote:
> Ruediger Pluem wrote on 2009-02-07 22:03:38:
>
>> IMHO this is correct. The problem is that we do not know at this
>> point of time how EnableSendFile is set. We are in the quick handler
>> and have not done any directory walks (and in fact if the cached
>> entry is good we never will).
>> So the only option I see here is to add another directive for
>> mod_disk_cache to determine what should be done.
>
> Well, but if EnableSendfile is configured in the main config
> or vhost we get the setting by looking at the core_module config.
> Of course you are right that this doesn't work when it is defined
> inside a Location or Directory section.
>
> We could just add a note to the mod_disk_cache configuration that
> EnableSendfile will only be taken into account when configured
> globally for the server or vhost. IMHO that's good enough for
> such a special case. I would like to avoid a dedicated sendfile
> directive just for mod_disk_cache.
>
> ciao...
+1
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Brian Akins <br...@akins.org>.
On 2/16/09 5:06 AM, "Niklas Edmundsson" <ni...@acc.umu.se> wrote:
>>> + core_dir_config *coreconf = ap_get_module_config(r->per_dir_config,
>>> + &core_module);
This is a perfect example of why we need a call to hide core_module stuff
from modules. We talked about this before and we are still propagating
this, IMO, bad habit.
--bakins
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Niklas Edmundsson <ni...@acc.umu.se>.
On Sun, 8 Feb 2009, Ruediger Pluem wrote:
>> Yes, it is. The following seems to work fine:
>>
>> Index: mod_disk_cache.c
>> ===================================================================
>> --- mod_disk_cache.c (revision 742187)
>> +++ mod_disk_cache.c (working copy)
>> @@ -471,7 +471,10 @@
>> /* Open the data file */
>> flags = APR_READ|APR_BINARY;
>> #ifdef APR_SENDFILE_ENABLED
>> - flags |= APR_SENDFILE_ENABLED;
>> + core_dir_config *coreconf = ap_get_module_config(r->per_dir_config,
>> + &core_module);
>
> This is not ANSI C compatible. Please declare coreconf at the
> beginning of the block. Otherwise no objections.
+1 when ansified.
/Nikke
--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Niklas Edmundsson, Admin @ {acc,hpc2n}.umu.se | nikke@acc.umu.se
---------------------------------------------------------------------------
To err is human, to forgive is unusual.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/08/2009 11:38 PM, Lars Eilebrecht wrote:
> Ruediger Pluem wrote on 2009-02-08 22:36:47:
>
>>> Well, but if EnableSendfile is configured in the main config
>>> or vhost we get the setting by looking at the core_module config.
>>> Of course you are right that this doesn't work when it is defined
>>> inside a Location or Directory section.
>> Well I am not sure if the core_dir_config structure is already setup
>> during the quick handler phase (even for settings done on VHOST
>> level).
>
> Yes, it is. The following seems to work fine:
>
> Index: mod_disk_cache.c
> ===================================================================
> --- mod_disk_cache.c (revision 742187)
> +++ mod_disk_cache.c (working copy)
> @@ -471,7 +471,10 @@
> /* Open the data file */
> flags = APR_READ|APR_BINARY;
> #ifdef APR_SENDFILE_ENABLED
> - flags |= APR_SENDFILE_ENABLED;
> + core_dir_config *coreconf = ap_get_module_config(r->per_dir_config,
> + &core_module);
This is not ANSI C compatible. Please declare coreconf at the beginning of the
block. Otherwise no objections.
Regards
Rüdiger
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Lars Eilebrecht <la...@eilebrecht.net>.
Ruediger Pluem wrote on 2009-02-08 22:36:47:
> > Well, but if EnableSendfile is configured in the main config
> > or vhost we get the setting by looking at the core_module config.
> > Of course you are right that this doesn't work when it is defined
> > inside a Location or Directory section.
>
> Well I am not sure if the core_dir_config structure is already setup
> during the quick handler phase (even for settings done on VHOST
> level).
Yes, it is. The following seems to work fine:
Index: mod_disk_cache.c
===================================================================
--- mod_disk_cache.c (revision 742187)
+++ mod_disk_cache.c (working copy)
@@ -471,7 +471,10 @@
/* Open the data file */
flags = APR_READ|APR_BINARY;
#ifdef APR_SENDFILE_ENABLED
- flags |= APR_SENDFILE_ENABLED;
+ core_dir_config *coreconf = ap_get_module_config(r->per_dir_config,
+ &core_module);
+ flags |= ((coreconf->enable_sendfile == ENABLE_SENDFILE_OFF)
+ ? 0 : APR_SENDFILE_ENABLED);
#endif
rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
if (rc != APR_SUCCESS) {
Unless this gets a -1 from anyone I'll commit this to trunk
together with an appropriate note in the documentation.
ciao...
--
Lars Eilebrecht
lars@eilebrecht.net
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/08/2009 09:35 PM, Lars Eilebrecht wrote:
> Ruediger Pluem wrote on 2009-02-07 22:03:38:
>
>> IMHO this is correct. The problem is that we do not know at this
>> point of time how EnableSendFile is set. We are in the quick handler
>> and have not done any directory walks (and in fact if the cached
>> entry is good we never will).
>> So the only option I see here is to add another directive for
>> mod_disk_cache to determine what should be done.
>
> Well, but if EnableSendfile is configured in the main config
> or vhost we get the setting by looking at the core_module config.
> Of course you are right that this doesn't work when it is defined
> inside a Location or Directory section.
Well I am not sure if the core_dir_config structure is already setup
during the quick handler phase (even for settings done on VHOST level).
Regards
Rüdiger
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Lars Eilebrecht <la...@eilebrecht.net>.
Ruediger Pluem wrote on 2009-02-07 22:03:38:
> IMHO this is correct. The problem is that we do not know at this
> point of time how EnableSendFile is set. We are in the quick handler
> and have not done any directory walks (and in fact if the cached
> entry is good we never will).
> So the only option I see here is to add another directive for
> mod_disk_cache to determine what should be done.
Well, but if EnableSendfile is configured in the main config
or vhost we get the setting by looking at the core_module config.
Of course you are right that this doesn't work when it is defined
inside a Location or Directory section.
We could just add a note to the mod_disk_cache configuration that
EnableSendfile will only be taken into account when configured
globally for the server or vhost. IMHO that's good enough for
such a special case. I would like to avoid a dedicated sendfile
directive just for mod_disk_cache.
ciao...
--
Lars Eilebrecht
lars@eilebrecht.net
Re: use of APR_SENDFILE_ENABLED in mod_disk_cache
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/07/2009 09:02 PM, Lars Eilebrecht wrote:
> Hi,
>
> mod_disk_cache uses the following in open_entity():
>
> #ifdef APR_SENDFILE_ENABLED
> flags |= APR_SENDFILE_ENABLED;
> #endif
> rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
>
> Maybe I'm getting confused with the various APR_SENDFILE
> defines, but shouldn't we be checking for the setting of
> the EnableSendFile directive as well?
> If httpd was build on a platform providing sendfile support
> then I guess APR_SENDFILE_ENABLED is set and we would always
> be using sendfile when serving files from the cache,
> regardless of the setting of the EnableSendFile directive.
IMHO this is correct. The problem is that we do not know at this
point of time how EnableSendFile is set. We are in the quick handler
and have not done any directory walks (and in fact if the cached
entry is good we never will).
So the only option I see here is to add another directive for
mod_disk_cache to determine what should be done.
Regards
Rüdiger