On 12/08/2007 07:31 PM, Jim Jagielski wrote:
>>
>> +static apr_status_t socket_cleanup(proxy_conn_rec *conn)
>> +{
>> + if (conn->sock) {
>> + apr_socket_close(conn->sock);
>> + conn->sock = NULL;
>> + }
>> + if (conn->connection) {
>> + apr_pool_cleanup_kill(conn->pool, conn, connection_cleanup);
>> + conn->connection = NULL;
>> + }
>> + return APR_SUCCESS;
>> +}
>> +
>
> Why not simply void? We don't check the return value
> and I see no way to return non-success anyway :)
I have done so in preparation for the next patch I have in mind to
avoid memory leakage (this is not connected to SSL / non SSL).
But as this function is not exposed so far I don't really care.
I will have a look later if I can convert this to void.
>>
>> Index: modules/proxy/mod_proxy_http.c
>>
>> + if (is_ssl) {
>> + ap_proxy_ssl_connection_cleanup(backend, r);
>> + }
>> +
>> /* Step One: Determine Who To Connect To */
>> if ((status = ap_proxy_determine_connection(p, r, conf, worker,
>> backend,
>>
>
> I assume all the below is to allow non HTTP to also honor SSL
> connections, otherwise we'd simply place the function in mod_proxy_http.c
> and not worry about the API issues... +1
>From a quick glance I thought that there are functions in proxy_util.c that are
only used by mod_proxy_http. But in fact ap_proxy_connection_create is also used
by mod_proxy_ftp. I am not sure if ap_proxy_ssl_connection_cleanup could be
useful
for mod_proxy_ftp. On the other hand if I do not put
ap_proxy_ssl_connection_cleanup
in proxy_util.c I need to export socket_cleanup from proxy_util.c which really
belongs there. So I would have the same API issue :-).
I found ap_proxy_determine_connection to be more worth of exposure than
socket_cleanup.
But finally it doesn't really matter.
BTW: I have not tested with FTP proxing so far.
Regards
Rüdiger
