On 06/27/2007 05:51 PM, Jim Jagielski wrote: > > On Jun 27, 2007, at 11:08 AM, Nick Kew wrote: > >> On Wed, 27 Jun 2007 14:17:36 -0000 >> [EMAIL PROTECTED] wrote: >> >>> + * mod_proxy: Arrange the timeout handling. >>> + Trunk version of patch: >>> + http://svn.apache.org/viewvc?view=rev&revision=550514 >>> + http://svn.apache.org/viewvc?view=rev&revision=546128 >>> + +1: jfclere >> >> Looks reasonable, but is there a reference to the problem it solves? >> >>> + >>> + * mod_proxy: Improve traces in ap_proxy_http_process_response() >>> + to investigate PR37770. >>> + Trunk version of patch: >>> + http://svn.apache.org/viewvc?view=rev&rev=549420 >>> + +1: jfclere >> >> Hmmm. This is designed to improve an error message? >> >> + tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc); >> + rv = ap_rgetline(&tmp_s, n, &len, r, fold, tmp_bb); >> + apr_brigade_destroy(tmp_bb); >> >> Isn't a whole new brigade an unnecessarily overhead (and >> potentially large if the function gets used more in future)? >> What problem does it solve? >> > > Yeah... all this just so we can see the return val > of ap_rgetline()??
Yes, but have a look at ap_getline in protocol.c which was used before instead of ap_proxygetline. It does exactly the same thing regarding the brigade. So his solution is not less efficient than the old one. Of course it can be discussed whether the old one was efficient enough :-). I guess we can create a brigade for this purpose at the beginning of ap_proxy_http_process_response pass it to ap_proxygetline and do a apr_brigade_cleanup each time after we have called ap_proxygetline. But in this case we can also use ap_rgetline directly in ap_proxy_http_process_response as there is not much left of ap_proxygetline in this case. The other aspect of this patch is the matter of code duplication. The question is whether it is good to nearly duplicate the code of ap_getline and thus have to manage it twice in different locations. Regards Ru"diger
