Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
A naming discussion! \o/ AP_BUCKET_IS_QUANTUM ~icing > Am 03.04.2020 um 21:27 schrieb Marion & Christophe JAILLET > : > > > Le 03/04/2020 à 20:41, Ruediger Pluem a écrit : >> I am undecided. I am not entirely convinced that it is a good name although >> all you said above is true, but on the other hand it >> is not that bad that I see a need to spend a lot of further time on this >> name searching topic :-). So +0. >> What do others think about the name? >> >> Regards >> >> Rüdiger > > My 2c, on all proposals (and 2 variations): > > -1: > AP_BUCKET_IS_MORPHING > AP_BUCKET_IS_OPAQUE > AP_BUCKET_DETERMINATE_LENGTH > > +1: > AP_BUCKET_HAS_LENGTH > AP_BUCKET_HAS_UNKNOWN_LENGTH > AP_BUCKET_UNKNOWN_LENGTH > AP_BUCKET_LENGTH_UNKNOWN > > CJ >
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
Le 03/04/2020 à 20:41, Ruediger Pluem a écrit : I am undecided. I am not entirely convinced that it is a good name although all you said above is true, but on the other hand it is not that bad that I see a need to spend a lot of further time on this name searching topic :-). So +0. What do others think about the name? Regards Rüdiger My 2c, on all proposals (and 2 variations): -1: AP_BUCKET_IS_MORPHING AP_BUCKET_IS_OPAQUE AP_BUCKET_DETERMINATE_LENGTH +1: AP_BUCKET_HAS_LENGTH AP_BUCKET_HAS_UNKNOWN_LENGTH AP_BUCKET_UNKNOWN_LENGTH AP_BUCKET_LENGTH_UNKNOWN CJ
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
On 4/3/20 6:39 PM, Yann Ylavic wrote:
> On Thu, Apr 2, 2020 at 11:28 AM Ruediger Pluem wrote:
>>
>> So how about
>>
>> #define AP_BUCKET_HAS_UNKNOWN_LENGTH(e) ((e)->length == (apr_size_t)-1)
>
> Still quite mouthful..
>
> What about? :
> /**
> * Determine if a bucket is opaque, i.e. of unknown data length and type
> * @param e The bucket to inspect
> * @return true or false
> */
> #define AP_BUCKET_IS_OPAQUE(e) ((e)->length == (apr_size_t)-1)
>
> Unlike FILE buckets with struct apr_bucket_file where we can get the
> ->fd, PIPE and SOCKET buckets don't expose their data type in the API,
> relying on (apr_{file,socket}_t *)e->data is not really API/ABI safe
> (though APR is unlikely to change that). I mean, while buckets are
> generally quite opaque and should be manipulated with the API methods,
> these "->length == -1" once are particularly opaque :)
I am undecided. I am not entirely convinced that it is a good name although all
you said above is true, but on the other hand it
is not that bad that I see a need to spend a lot of further time on this name
searching topic :-). So +0.
What do others think about the name?
Regards
Rüdiger
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
On Thu, Apr 2, 2020 at 11:28 AM Ruediger Pluem wrote:
>
> So how about
>
> #define AP_BUCKET_HAS_UNKNOWN_LENGTH(e) ((e)->length == (apr_size_t)-1)
Still quite mouthful..
What about? :
/**
* Determine if a bucket is opaque, i.e. of unknown data length and type
* @param e The bucket to inspect
* @return true or false
*/
#define AP_BUCKET_IS_OPAQUE(e) ((e)->length == (apr_size_t)-1)
Unlike FILE buckets with struct apr_bucket_file where we can get the
->fd, PIPE and SOCKET buckets don't expose their data type in the API,
relying on (apr_{file,socket}_t *)e->data is not really API/ABI safe
(though APR is unlikely to change that). I mean, while buckets are
generally quite opaque and should be manipulated with the API methods,
these "->length == -1" once are particularly opaque :)
>
> It should be in APR, but that takes quite long until it arrives in httpd. So
> we might create one here and in APR in parallel
> and define the AP_ conditionally as the APR_ one if it exists and otherwise
> on our own.
+1
Regards,
Yann.
Index: include/http_request.h
===
--- include/http_request.h (revision 1876034)
+++ include/http_request.h (working copy)
@@ -589,13 +589,11 @@ AP_DECLARE(int) ap_if_walk(request_rec *r);
AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_eor;
/**
- * Determine if a bucket is morphing, that is which changes its
- * type on read (usually to "heap" allocated data), while moving
- * itself at the next position to remain plugged until exhausted.
+ * Determine if a bucket is opaque, i.e. of unknown data type and length
* @param e The bucket to inspect
* @return true or false
*/
-#define AP_BUCKET_IS_MORPHING(e)((e)->length == (apr_size_t)-1)
+#define AP_BUCKET_IS_OPAQUE(e) ((e)->length == (apr_size_t)-1)
/**
* Determine if a bucket is an End Of REQUEST (EOR) bucket
Index: modules/cache/mod_cache.c
===
--- modules/cache/mod_cache.c (revision 1876034)
+++ modules/cache/mod_cache.c (working copy)
@@ -1281,7 +1281,7 @@ static apr_status_t cache_save_filter(ap_filter_t
if (APR_BUCKET_IS_FLUSH(e)) {
continue;
}
-if (e->length == (apr_size_t)-1) {
+if (AP_BUCKET_IS_OPAQUE(e)) {
break;
}
size += e->length;
Index: modules/http/byterange_filter.c
===
--- modules/http/byterange_filter.c (revision 1876034)
+++ modules/http/byterange_filter.c (working copy)
@@ -435,7 +435,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_byterange_
*/
for (e = APR_BRIGADE_FIRST(bb);
(e != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(e)
- && e->length != (apr_size_t)-1);
+ && !AP_BUCKET_IS_OPAQUE(e));
e = APR_BUCKET_NEXT(e)) {
clength += e->length;
}
Index: modules/http/chunk_filter.c
===
--- modules/http/chunk_filter.c (revision 1876034)
+++ modules/http/chunk_filter.c (working copy)
@@ -88,7 +88,7 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f,
}
break;
}
-else if (e->length == (apr_size_t)-1) {
+else if (AP_BUCKET_IS_OPAQUE(e)) {
/* unknown amount of data (e.g. a pipe) */
const char *data;
apr_size_t len;
Index: modules/http2/h2_stream.c
===
--- modules/http2/h2_stream.c (revision 1876034)
+++ modules/http2/h2_stream.c (working copy)
@@ -21,6 +21,7 @@
#include
#include
+#include
#include
#include
@@ -866,7 +867,7 @@ static apr_status_t add_buffered_data(h2_stream *s
apr_bucket_destroy(b);
}
else {
-ap_assert(b->length != (apr_size_t)-1);
+ap_assert(!AP_BUCKET_IS_OPAQUE(b));
*plen += b->length;
if (*plen >= requested) {
*plen = requested;
Index: server/core_filters.c
===
--- server/core_filters.c (revision 1876034)
+++ server/core_filters.c (working copy)
@@ -277,7 +277,7 @@ apr_status_t ap_core_input_filter(ap_filter_t *f,
while ((len < readbytes) && (rv == APR_SUCCESS)
&& (e != APR_BRIGADE_SENTINEL(ctx->bb))) {
/* Check for the availability of buckets with known length */
-if (e->length != (apr_size_t)-1) {
+if (!AP_BUCKET_IS_OPAQUE(e)) {
len += e->length;
e = APR_BUCKET_NEXT(e);
}
Index: server/protocol.c
===
--- server/protocol.c (revision 1876034)
+++ server/protocol.c (working copy)
@@ -1844,7
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
On 4/2/20 11:14 AM, Joe Orton wrote: > On Thu, Apr 02, 2020 at 10:58:21AM +0200, Yann Ylavic wrote: >> On Thu, Apr 2, 2020 at 6:39 AM Ruediger Pluem wrote: >>> +#define AP_BUCKET_IS_MORPHING(e)((e)->length == (apr_size_t)-1) >>> >>> Nitpick: After having a second thought on the whole thing, I think the >>> above name is misleading to some extend. If MMAP is enabled >>> a file bucket is also a morphing bucket as a read on it causes the bucket >>> to split in an MMAP bucket and a shorter file bucket. >>> A MMAP bucket consumes at least address space and I vaguely remember cases >>> from the past (back in 32 bit times) where filters that >>> processed (doing apr_bucket_read) a whole brigade at once without passing >>> results down the chain on a regular basis caused the >>> address space to be exhausted by MMAP buckets if the file bucket was huge. >> >> How about a less subjective: >> #define AP_BUCKET_HAS_LENGTH(e) ((e)->length != (apr_size_t)-1) != is bad as we need == at least in the cases you mentioned and hence we had to do !AP_BUCKET_HAS_LENGTH(e). Not sure if the compiler optimizes it away. So how about #define AP_BUCKET_HAS_UNKNOWN_LENGTH(e) ((e)->length == (apr_size_t)-1) But as said below: It doesn't save characters. It might be easier to read, to memorize and less error prone as the cast cannot be forgotten :-). >> ? >> >> If find it quite painful to type e->length == (apr_size_t)-1, though I >> could simply use -1 and rely on unsigned promotion.. > > Formally this is *determinate* length - all buckets have a length just > some of them are indeterminate. > > #define AP_BUCKET_DETERMINATE_LENGTH(e) ((e)->length != (apr_size_t)-1) > > but it is quite a mouthful and exactly the same number of characters as > writing it out longhand. No strong opinion on having a macro for it, > probably should be in APR tho ;) > It should be in APR, but that takes quite long until it arrives in httpd. So we might create one here and in APR in parallel and define the AP_ conditionally as the APR_ one if it exists and otherwise on our own. Regards Rüdiger
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
On Thu, Apr 02, 2020 at 10:58:21AM +0200, Yann Ylavic wrote: > On Thu, Apr 2, 2020 at 6:39 AM Ruediger Pluem wrote: > > > > > +#define AP_BUCKET_IS_MORPHING(e)((e)->length == (apr_size_t)-1) > > > > Nitpick: After having a second thought on the whole thing, I think the > > above name is misleading to some extend. If MMAP is enabled > > a file bucket is also a morphing bucket as a read on it causes the bucket > > to split in an MMAP bucket and a shorter file bucket. > > A MMAP bucket consumes at least address space and I vaguely remember cases > > from the past (back in 32 bit times) where filters that > > processed (doing apr_bucket_read) a whole brigade at once without passing > > results down the chain on a regular basis caused the > > address space to be exhausted by MMAP buckets if the file bucket was huge. > > How about a less subjective: > #define AP_BUCKET_HAS_LENGTH(e) ((e)->length != (apr_size_t)-1) > ? > > If find it quite painful to type e->length == (apr_size_t)-1, though I > could simply use -1 and rely on unsigned promotion.. Formally this is *determinate* length - all buckets have a length just some of them are indeterminate. #define AP_BUCKET_DETERMINATE_LENGTH(e) ((e)->length != (apr_size_t)-1) but it is quite a mouthful and exactly the same number of characters as writing it out longhand. No strong opinion on having a macro for it, probably should be in APR tho ;)
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
On Thu, Apr 2, 2020 at 6:39 AM Ruediger Pluem wrote: > > > +#define AP_BUCKET_IS_MORPHING(e)((e)->length == (apr_size_t)-1) > > Nitpick: After having a second thought on the whole thing, I think the above > name is misleading to some extend. If MMAP is enabled > a file bucket is also a morphing bucket as a read on it causes the bucket to > split in an MMAP bucket and a shorter file bucket. > A MMAP bucket consumes at least address space and I vaguely remember cases > from the past (back in 32 bit times) where filters that > processed (doing apr_bucket_read) a whole brigade at once without passing > results down the chain on a regular basis caused the > address space to be exhausted by MMAP buckets if the file bucket was huge. How about a less subjective: #define AP_BUCKET_HAS_LENGTH(e) ((e)->length != (apr_size_t)-1) ? If find it quite painful to type e->length == (apr_size_t)-1, though I could simply use -1 and rely on unsigned promotion.. Regards, Yann.
Re: svn commit: r1875947 - in /httpd/httpd/trunk: include/http_request.h modules/http/http_core.c modules/http/http_request.c server/core.c server/core_filters.c server/request.c server/util_filter.c
On 3/31/20 6:22 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Tue Mar 31 16:22:53 2020
> New Revision: 1875947
>
> URL: http://svn.apache.org/viewvc?rev=1875947=rev
> Log:
> core: handle morphing buckets setaside/reinstate and kill request core filter.
>
> The purpose of ap_request_core_filter() is not clear, it seems to prevent
> potential morphing buckets to go through AP_FTYPE_CONNECTION filters which
> would fail to set them aside (ENOTIMPL), and read them (unbounded) in memory.
>
> This patch allows ap_filter_setaside_brigade() to set morphing buckets aside
> by simply moving them, assuming they have the correct lifetime (either until
> some further EOR, or the connection lifetime, or whatever). IOW, the module is
> responsible for sending morphing buckets whose lifetime needs not be changed
> by the connection filters.
>
> Now since morphing buckets consume no memory until (apr_bucket_)read, like
> FILE
> buckets, we don't account for them in flush_max_threshold either. This changes
> ap_filter_reinstate_brigade() to only account for in-memory and EOR buckets to
> flush_upto.
>
> Also, since the EOR bucket is sent only to c->output_filters once the request
> is processed, when all the filters < AP_FTYPE_CONNECTION have done their job
> and stopped retaining data (after the EOS bucket, if ever), we prevent misuse
> of ap_filter_{setaside,reinstate}_brigade() outside connection filters by
> returning ENOTIMPL. This is not the right API for request filters as of now.
>
> Finally, ap_request_core_filter() and co can be removed.
>
> Modified:
> httpd/httpd/trunk/include/http_request.h
> httpd/httpd/trunk/modules/http/http_core.c
> httpd/httpd/trunk/modules/http/http_request.c
> httpd/httpd/trunk/server/core.c
> httpd/httpd/trunk/server/core_filters.c
> httpd/httpd/trunk/server/request.c
> httpd/httpd/trunk/server/util_filter.c
>
> Modified: httpd/httpd/trunk/include/http_request.h
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_request.h?rev=1875947=1875946=1875947=diff
> ==
> --- httpd/httpd/trunk/include/http_request.h (original)
> +++ httpd/httpd/trunk/include/http_request.h Tue Mar 31 16:22:53 2020
> @@ -601,6 +589,15 @@ AP_DECLARE(int) ap_if_walk(request_rec *
> AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_eor;
>
> /**
> + * Determine if a bucket is morphing, that is which changes its
> + * type on read (usually to "heap" allocated data), while moving
> + * itself at the next position to remain plugged until exhausted.
> + * @param e The bucket to inspect
> + * @return true or false
> + */
> +#define AP_BUCKET_IS_MORPHING(e)((e)->length == (apr_size_t)-1)
Nitpick: After having a second thought on the whole thing, I think the above
name is misleading to some extend. If MMAP is enabled
a file bucket is also a morphing bucket as a read on it causes the bucket to
split in an MMAP bucket and a shorter file bucket.
A MMAP bucket consumes at least address space and I vaguely remember cases from
the past (back in 32 bit times) where filters that
processed (doing apr_bucket_read) a whole brigade at once without passing
results down the chain on a regular basis caused the
address space to be exhausted by MMAP buckets if the file bucket was huge.
> +
> +/**
> * Determine if a bucket is an End Of REQUEST (EOR) bucket
> * @param e The bucket to inspect
> * @return true or false
>
Regards
Rüdiger
