You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Akins, Brian" <Br...@turner.com> on 2008/09/25 20:08:00 UTC
[PATCH] add ap_sendfile_enabled
Got tired of having to set core_private for one bit of info. How does this
look?
Index: server/request.c
===================================================================
--- server/request.c (revision 699033)
+++ server/request.c (working copy)
@@ -1907,3 +1907,14 @@
return (r->main == NULL) /* otherwise, this is a sub-request */
&& (r->prev == NULL); /* otherwise, this is an internal
redirect */
}
+
+AP_DECLARE(int) ap_sendfile_enabled(request_rec *r)
+{
+ int rc = 0;
+#if APR_HAS_SENDFILE
+ core_dir_config *conf = ap_get_module_config(r->per_dir_config,
+ &core_module);
+ rc = conf->enable_sendfile == ENABLE_SENDFILE_OFF;
+#endif
+ return rc;
+}
Index: include/httpd.h
===================================================================
--- include/httpd.h (revision 699033)
+++ include/httpd.h (working copy)
@@ -1541,6 +1541,13 @@
AP_DECLARE(int) ap_count_dirs(const char *path);
/**
+ * Is sendfile enabled for this request
+ * @param r the current request
+ * @return 1 if enabled, 0 is not
+ */
+AP_DECLARE(int) ap_sendfile_enabled(request_rec *r);
+
+/**
* Copy at most @a n leading directories of @a s into @a d. @a d
* should be at least as large as @a s plus 1 extra byte
*
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: [PATCH] add ap_sendfile_enabled
Posted by Paul Querna <ch...@force-elite.com>.
William A. Rowe, Jr. wrote:
> Akins, Brian wrote:
>> On 9/25/08 2:24 PM, "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>>
>>> Hmmm... -0.9, it's private because the connection mechanics are private.
>>> Core transport is part of core.
>>>
>>> What was the rational?
>> Bcs lots of modules have things like this. Using mod_dialup as an example:
>
> Of course [duh!]
>
> But it folds back to another issue... for a long time we've chattered that
> holding the apr_file_t to the designated resource, opened before-or-during
> the directory_walk and forever tested from that point on using it's handle
> instead of it's name would be optimal.
>
> If we finally s&*t and get off the pot to add this in 2.4 / 3.0, and this
> was determined by core which -also- opened the target file (opening it
> appropriately to enable sendfile, of course), then where does that leave
> us? Do we still seek this new api call?
Yes, I think we should add this API.
Pulling in CORE_PRIVATE is extremely lame, but its currently the only
way to tell if a File Path has sendfile enabled or disabled. It should
be an API.
I also wouldn't mind a API that took a char* path, and a request_rec,
because someitmes you are inserting buckets from different files, with
different paths, and the file path in the main request record isn't
always accurate.
-Paul
Re: [PATCH] add ap_sendfile_enabled
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Akins, Brian wrote:
> On 9/25/08 2:24 PM, "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>
>> Hmmm... -0.9, it's private because the connection mechanics are private.
>> Core transport is part of core.
>>
>> What was the rational?
>
> Bcs lots of modules have things like this. Using mod_dialup as an example:
Of course [duh!]
But it folds back to another issue... for a long time we've chattered that
holding the apr_file_t to the designated resource, opened before-or-during
the directory_walk and forever tested from that point on using it's handle
instead of it's name would be optimal.
If we finally s&*t and get off the pot to add this in 2.4 / 3.0, and this
was determined by core which -also- opened the target file (opening it
appropriately to enable sendfile, of course), then where does that leave
us? Do we still seek this new api call?
Bill
Re: [PATCH] add ap_sendfile_enabled
Posted by "Akins, Brian" <Br...@turner.com>.
On 9/25/08 2:24 PM, "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
> Hmmm... -0.9, it's private because the connection mechanics are private.
> Core transport is part of core.
>
> What was the rational?
Bcs lots of modules have things like this. Using mod_dialup as an example:
/* to detect sendfile enabled, we need CORE_PRIVATE. Someone should fix
this. */
#define CORE_PRIVATE
#include "http_core.h"
...
core_dir_config *ccfg = ap_get_module_config(r->per_dir_config,
&core_module);
...
rv = apr_file_open(&fd, r->filename, APR_READ | APR_BINARY
#if APR_HAS_SENDFILE
| ((ccfg->enable_sendfile == ENABLE_SENDFILE_OFF)
? 0 : APR_SENDFILE_ENABLED)
#endif
, 0, r->pool);
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: [PATCH] add ap_sendfile_enabled
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Hmmm... -0.9, it's private because the connection mechanics are private.
Core transport is part of core.
What was the rational?
Akins, Brian wrote:
> Got tired of having to set core_private for one bit of info. How does this
> look?
>
>
> Index: server/request.c
> ===================================================================
> --- server/request.c (revision 699033)
> +++ server/request.c (working copy)
> @@ -1907,3 +1907,14 @@
> return (r->main == NULL) /* otherwise, this is a sub-request */
> && (r->prev == NULL); /* otherwise, this is an internal
> redirect */
> }
> +
> +AP_DECLARE(int) ap_sendfile_enabled(request_rec *r)
> +{
> + int rc = 0;
> +#if APR_HAS_SENDFILE
> + core_dir_config *conf = ap_get_module_config(r->per_dir_config,
> + &core_module);
> + rc = conf->enable_sendfile == ENABLE_SENDFILE_OFF;
> +#endif
> + return rc;
> +}
> Index: include/httpd.h
> ===================================================================
> --- include/httpd.h (revision 699033)
> +++ include/httpd.h (working copy)
> @@ -1541,6 +1541,13 @@
> AP_DECLARE(int) ap_count_dirs(const char *path);
>
> /**
> + * Is sendfile enabled for this request
> + * @param r the current request
> + * @return 1 if enabled, 0 is not
> + */
> +AP_DECLARE(int) ap_sendfile_enabled(request_rec *r);
> +
> +/**
> * Copy at most @a n leading directories of @a s into @a d. @a d
> * should be at least as large as @a s plus 1 extra byte
> *
>
>
Re: [PATCH] add ap_sendfile_enabled
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Sep 30, 2008, at 1:15 PM, William A. Rowe, Jr. wrote:
>
> Rather than many symbols that become slow to bind, would it make
> more sense
> to either pull aside the core config stuff so that anyone with a
> legitimate
> reason can inspect it all without CORE_PRIVATE_EVERYTHING, or should
> we look
> at something closer to ap_mpm_query (ap_core_query?) and start
> adding it a
> whole lot of stuff?
>
Me like.
> Something to ponder; injecting this as-is for 2.2.10 seems rushed.
>
?? Who said anything about this being in 2.2.10?
Re: [PATCH] add ap_sendfile_enabled
Posted by "Akins, Brian" <Br...@turner.com>.
On 9/30/08 1:15 PM, "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
> should we look
> at something closer to ap_mpm_query (ap_core_query?) and start adding it a
> whole lot of stuff?
+1
> Something to ponder; injecting this as-is for 2.2.10 seems rushed.
+1
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: [PATCH] add ap_sendfile_enabled
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Jim Jagielski wrote:
>
> On Sep 29, 2008, at 9:20 AM, Akins, Brian wrote:
>
>> We need to do something or users - including ourselves - will do the very
>> bad thing of declaring CORE_PRIVATE all over the place.
>>
>
> +1...
But; is replicating the entire API really a solution? Because for every
obvious case (and your concern is a good one) someone wants to use a more
obscure case (and can back it up with a legitimate case).
Rather than many symbols that become slow to bind, would it make more sense
to either pull aside the core config stuff so that anyone with a legitimate
reason can inspect it all without CORE_PRIVATE_EVERYTHING, or should we look
at something closer to ap_mpm_query (ap_core_query?) and start adding it a
whole lot of stuff?
Something to ponder; injecting this as-is for 2.2.10 seems rushed.
Re: [PATCH] add ap_sendfile_enabled
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Sep 29, 2008, at 9:20 AM, Akins, Brian wrote:
> On 9/25/08 9:37 PM, "William A. Rowe, Jr." <wr...@rowe-clan.net>
> wrote:
>>
>> Couple of points, API fn name sucks, can it be
>> ap_request_config_test(r,feat)?
>
> Sure.
>
>> Always request-rec based, there should be no need for server rec
>> (two args
>> are faster than 3).
>
> True.
>
>> About your other idea, it's a no-go.
>
>
> For the record, it was Paul's idea to add a const char* arg...
>
>
> We need to do something or users - including ourselves - will do the
> very
> bad thing of declaring CORE_PRIVATE all over the place.
>
+1...
Re: [PATCH] add ap_sendfile_enabled
Posted by "Akins, Brian" <Br...@turner.com>.
On 9/25/08 9:37 PM, "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>
> Couple of points, API fn name sucks, can it be ap_request_config_test(r,feat)?
Sure.
> Always request-rec based, there should be no need for server rec (two args
> are faster than 3).
True.
> About your other idea, it's a no-go.
For the record, it was Paul's idea to add a const char* arg...
We need to do something or users - including ourselves - will do the very
bad thing of declaring CORE_PRIVATE all over the place.
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: [PATCH] add ap_sendfile_enabled
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Akins, Brian wrote:
>
> int ap_feature_enabled(request_rec *r, server_rec *s, int features)
>
> if(ap_feature_enabled(r, NULL, SENDFILE)) {
> blah...
> }
Couple of points, API fn name sucks, can it be ap_request_config_test(r,feat)?
Always request-rec based, there should be no need for server rec (two args
are faster than 3).
About your other idea, it's a no-go. You would have to perform a subrequest
to determine the entire config parsing, before then testing the feature.
It's so expensive, it's misleading to shorthand it into a single function.
But I don't know that you need to (for adding buckets from multiple files)
unless you want the added complexity of adding sendfile buckets.
Bill
Re: [PATCH] add ap_sendfile_enabled
Posted by "Akins, Brian" <Br...@turner.com>.
On 9/25/08 5:35 PM, "Basant Kumar kukreja" <Ba...@Sun.COM> wrote:
> With this common API, we can add more flags in future.
I'm personally -1 on this.
How about something like this:
int ap_feature_enabled(request_rec *r, server_rec *s, int features)
if(ap_feature_enabled(r, NULL, SENDFILE)) {
blah...
}
Or something so I can just query those things I care about. Like we do with
apr_stat, sorta.
--
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies
Re: [PATCH] add ap_sendfile_enabled
Posted by Basant Kumar kukreja <Ba...@Sun.COM>.
We can probably have a generic API which returns the enable/disable flags in
bit pattern e.g
AP_DECLARE(int) ap_feature_enabled(request_rec *r);
// Caller :
int features = ap_feature_enabled(r);
if (features & SENDFILE_FLAG) // send file is enabled
if (features & MMAP_FLAG) // mmap is enabled
With this common API, we can add more flags in future.
Regards,
Basant.
On Thu, Sep 25, 2008 at 02:08:00PM -0400, Akins, Brian wrote:
> Got tired of having to set core_private for one bit of info. How does this
> look?
>
>
> Index: server/request.c
> ===================================================================
> --- server/request.c (revision 699033)
> +++ server/request.c (working copy)
> @@ -1907,3 +1907,14 @@
> return (r->main == NULL) /* otherwise, this is a sub-request */
> && (r->prev == NULL); /* otherwise, this is an internal
> redirect */
> }
> +
> +AP_DECLARE(int) ap_sendfile_enabled(request_rec *r)
> +{
> + int rc = 0;
> +#if APR_HAS_SENDFILE
> + core_dir_config *conf = ap_get_module_config(r->per_dir_config,
> + &core_module);
> + rc = conf->enable_sendfile == ENABLE_SENDFILE_OFF;
> +#endif
> + return rc;
> +}
> Index: include/httpd.h
> ===================================================================
> --- include/httpd.h (revision 699033)
> +++ include/httpd.h (working copy)
> @@ -1541,6 +1541,13 @@
> AP_DECLARE(int) ap_count_dirs(const char *path);
>
> /**
> + * Is sendfile enabled for this request
> + * @param r the current request
> + * @return 1 if enabled, 0 is not
> + */
> +AP_DECLARE(int) ap_sendfile_enabled(request_rec *r);
> +
> +/**
> * Copy at most @a n leading directories of @a s into @a d. @a d
> * should be at least as large as @a s plus 1 extra byte
> *
>
>
> --
> Brian Akins
> Chief Operations Engineer
> Turner Digital Media Technologies
>