Re: svn commit: r1885573 - in /httpd/httpd/branches/2.4.x: CHANGES STATUS docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h modules/ssl/ssl_engine_io.c server/core.c server/core_filters.c
On Wed, Feb 3, 2021 at 8:56 AM Ruediger Pluem wrote: > > Another nitpick I came across when reviewing this patch: > > Shouldn't should_send_brigade return an int instead of apr_status_t? Done in http://home.apache.org/~ylavic/patches/follow_up_to_r1885239_and_r1885573-v2.patch (updated backport proposal). Thanks; Yann.
Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
On Wed, Feb 3, 2021 at 9:48 AM Ruediger Pluem wrote: > > On 2/3/21 1:23 AM, Yann Ylavic wrote: > > > > I think we can remove this test/block completely now that > > Agreed. I think we should just decline and leave it to further possible > protocol handlers to deal > with these URL's. And if all decline we just hit the catch all that there is > no handler for this URL. > So +1 to just remove the block. Will you do? r1886151. Regards; Yann.
Re: svn commit: r1886141 - /httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
On 2/3/21 1:23 AM, Yann Ylavic wrote:
> On Tue, Feb 2, 2021 at 8:50 PM wrote:
>>
>> Author: rpluem
>> Date: Tue Feb 2 19:50:14 2021
>> New Revision: 1886141
>>
> []
>> if (!scheme || u[0] != '/' || u[1] != '/' || u[2] == '\0') {
>> -if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
>> -ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
>> - "overlong proxy URL scheme in %s", url);
>> -return HTTP_BAD_REQUEST;
>> -}
>> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01113)
>>"HTTP: declining URL %s", url);
>> return DECLINED; /* only interested in HTTP, WS or FTP via proxy */
>> }
>> +if (!scheme && (u = strchr(url, ':')) && (u - url) > 14) {
>> +ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10262)
>> + "overlong proxy URL scheme in %s", url);
>> +return HTTP_BAD_REQUEST;
>> +}
>
> Hmm, actually this !scheme here is unreachable now.
Good catch.
> I think we can remove this test/block completely now that
Agreed. I think we should just decline and leave it to further possible
protocol handlers to deal
with these URL's. And if all decline we just hit the catch all that there is no
handler for this URL.
So +1 to just remove the block. Will you do?
Regards
Rüdiger
Re: svn commit: r1885605 - in /httpd/httpd/branches/2.4.x: ./ include/ modules/proxy/
On 2/2/21 4:18 PM, Yann Ylavic wrote:
> On Tue, Feb 2, 2021 at 10:32 AM Ruediger Pluem wrote:
>>
>>> New Revision: 1885605
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1885605=rev
> []
>>> +/* Yield if the output filters stack is full? This is to avoid
>>> + * blocking and give the caller a chance to POLLOUT async.
>>> + */
>>> +if (flags & AP_PROXY_TRANSFER_YIELD_PENDING) {
>>> +int rc = OK;
>>> +
>>> +if (!ap_filter_should_yield(c_o->output_filters)) {
>>> +rc = ap_filter_output_pending(c_o);
>>
>> I am confused here: !ap_filter_should_yield(c_o->output_filters) means there
>> is no pending output.
>> Why should we try to send it then? Shouldn't it be the other way round?
>
> Yes, !ap_filter_should_yield() means that there is no pending output,
> but only for filters that play the
> ap_filter_{setaside,reinstate}_brigade() game.
>
> The goal here is to try to flush pending data of filters that don't
> ap_filter_setaside_brigade(), like e.g. ssl_io_filter_coalesce() which
But isn't ap_filter_output_pending a noop that always returns DECLINED in case
of !ap_filter_should_yield(c_o->output_filters)?
Regards
Rüdiger
