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