see overall comments at the end
Randall Stewart <[EMAIL PROTECTED]> writes:
> call is made.... Yes most likely the kernel developers of the world
> WILL make sure that TCP is returned.. but there are no assurances
> that a programmer might make a mistake :-0
and when the kernel developers make such mistakes, APR apps like any
others will fall on the floor :)
> Index: configure.in
> ===================================================================
> RCS file: /home/cvspublic/apr/configure.in,v
> retrieving revision 1.484
> diff -u -r1.484 configure.in
> --- configure.in 3 Oct 2002 15:31:49 -0000 1.484
> +++ configure.in 13 Oct 2002 20:54:22 -0000
> @@ -974,7 +974,20 @@
> else
> AC_MSG_RESULT(no)
> fi
> -
> +AC_MSG_CHECKING(for netinet/sctp.h)
> +AC_TRY_CPP([
> +#ifdef HAVE_NETINET_IN_H
> +#include <sys/types.h>
> +#endif
> +#include <netinet/sctp.h>
> +], netinet_sctph="1", netinet_sctph="0")
> +if test $netinet_sctph = 1; then
> + AC_MSG_RESULT(yes)
> + echo "#define HAVE_NETINET_SCTP_H 1" >> confdefs.h
> +else
> + AC_MSG_RESULT(no)
> +fi
> +AC_SUBST(netinet_sctph)
Does sctp.h require that sys/types.h is already included? If so I
guess we need the complicated check like this. But check #ifdef
HAVE_SYS_TYPES_H prior to including sys/types instead of checking
HAVE_NETINET_IN_H
Until we see an implementation where <netinet/sctp.h> bombs without
<sys/types.h> included first we should use the simple check for the
header file (i.e., add something to APR_FLAG_HEADERS() invocation.
> Index: include/apr_network_io.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_network_io.h,v
> retrieving revision 1.129
> diff -u -r1.129 apr_network_io.h
> --- include/apr_network_io.h 11 Oct 2002 20:41:23 -0000 1.129
> +++ include/apr_network_io.h 13 Oct 2002 20:54:24 -0000
> @@ -271,7 +271,7 @@
> * @param cont The pool to use
> */
> APR_DECLARE(apr_status_t) apr_socket_create(apr_socket_t **new_sock,
> - int family, int type,
> + int family, int type, int
> protocol,
> apr_pool_t *cont);
yeah, this change needs to be made before APR hits 1.0
another thing:
probably need to create
APR_PROTO_TCP
APR_PROTO_UDP
APR_PROTO_SCTP
in this header file for portability... of course we know what numbers
are assigned to these protocols so no problem coming up with the values
> --- include/apr_portable.h 3 Oct 2002 17:55:42 -0000 1.81
> +++ include/apr_portable.h 13 Oct 2002 20:54:24 -0000
> @@ -214,6 +214,7 @@
> struct sockaddr *remote; /**< NULL if not connected */
> int family; /**< always required (APR_INET, APR_INET6, etc.
> */
> int type; /**< always required (SOCK_STREAM, SOCK_DGRAM,
> etc. */
> + int protocol; /** IPPROTO_SCTP/IPPROTO_TCP/IPPROTO_UDP **/
if we have our own APR_ constants, then those would show up in the
comment here... note your use of tabs and changing the comment
style... both issues should be fixed (other uses of tabs in your patch
that need to be remedied)
> Index: include/arch/unix/networkio.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/arch/unix/networkio.h,v
> retrieving revision 1.54
> diff -u -r1.54 networkio.h
> --- include/arch/unix/networkio.h 11 Jul 2002 05:19:44 -0000 1.54
> +++ include/arch/unix/networkio.h 13 Oct 2002 20:54:25 -0000
> @@ -87,6 +87,10 @@
> #if APR_HAVE_NETINET_TCP_H
> #include <netinet/tcp.h>
> #endif
> +#if APR_HAVE_NETINET_SCTP_H
> +#include <netinet/sctp_uio.h>
> +#include <netinet/sctp.h>
> +#endif
is it guaranteed that having sctp.h implies the other? otherwise,
make a check for both
> Index: include/arch/win32/networkio.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/arch/win32/networkio.h,v
> retrieving revision 1.28
> diff -u -r1.28 networkio.h
> --- include/arch/win32/networkio.h 15 Jul 2002 07:26:12 -0000 1.28
> +++ include/arch/win32/networkio.h 13 Oct 2002 20:54:25 -0000
> @@ -62,6 +62,7 @@
> apr_pool_t *cntxt;
> SOCKET socketdes;
> int type; /* SOCK_STREAM, SOCK_DGRAM */
> + int protocol;
tab :) (I promise not to mention it again)
> Index: network_io/unix/sockopt.c
> ===================================================================
> RCS file: /home/cvspublic/apr/network_io/unix/sockopt.c,v
> retrieving revision 1.59
> diff -u -r1.59 sockopt.c
> --- network_io/unix/sockopt.c 15 Jul 2002 20:29:38 -0000 1.59
> +++ network_io/unix/sockopt.c 13 Oct 2002 20:54:26 -0000
> @@ -231,12 +231,28 @@
> }
> if (opt & APR_TCP_NODELAY) {
> #if defined(TCP_NODELAY)
> +#if APR_HAVE_NETINET_SCTP_H
> + if (apr_is_option_set(sock->netmask, APR_TCP_NODELAY) != on){
need blank after ')' and before '{' (global comment)
> + if(sock->protocol == IPPROTO_SCTP){
need blank after 'if' and before '(' (global comment)
> + if (setsockopt(sock->socketdes, IPPROTO_SCTP, SCTP_NODELAY,
> (void *)&on, sizeof(int)) == -1) {
> + return errno;
> + }
> + }else{
"} else {"
> +#if APR_HAVE_NETINET_SCTP_H
checking for a header file instead of a feature just doesn't sit well
with me here
seems like configure should really ensure that the feature is
available, then set APR_HAVE_SCTP to 1 in apr.h, and code like this
should check
#if APR_HAVE_SCTP
> Index: network_io/win32/sockopt.c
> ===================================================================
> RCS file: /home/cvspublic/apr/network_io/win32/sockopt.c,v
> retrieving revision 1.45
> diff -u -r1.45 sockopt.c
> --- network_io/win32/sockopt.c 15 Jul 2002 20:29:38 -0000 1.45
> +++ network_io/win32/sockopt.c 13 Oct 2002 20:54:27 -0000
> @@ -193,6 +193,43 @@
> break;
> }
> case APR_TCP_NODELAY:
> +> #if APR_HAVE_NETINET_SCTP_H
what's with '>'?
> Index: test/client.c
> ===================================================================
> RCS file: /home/cvspublic/apr/test/client.c,v
> retrieving revision 1.36
> diff -u -r1.36 client.c
> --- test/client.c 15 Jul 2002 07:56:13 -0000 1.36
> +++ test/client.c 13 Oct 2002 20:54:28 -0000
> @@ -110,7 +110,7 @@
> fprintf(stdout,"OK\n");
>
> fprintf(stdout, "\tClient: Creating new socket.......");
> - if (apr_socket_create(&sock, remote_sa->family, SOCK_STREAM,
> + if (apr_socket_create(&sock, remote_sa->family, SOCK_STREAM,IPPROTO_TCP,
need blank between parms
this should use APR_PROTO_TCP instead of IPPROTO_TCP
(same comment for other test programs and Apache code)
> Index: server/listen.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/server/listen.c,v
> retrieving revision 1.82
> diff -u -r1.82 listen.c
> --- server/listen.c 31 Jul 2002 12:44:55 -0000 1.82
> +++ server/listen.c 14 Oct 2002 11:43:07 -0000
> @@ -229,7 +229,9 @@
> apr_socket_t *tmp_sock;
> apr_sockaddr_t *sa;
>
> - if ((sock_rv = apr_socket_create(&tmp_sock, APR_INET6, SOCK_STREAM,
> p))
> + if ((sock_rv = apr_socket_create(&tmp_sock, APR_INET6, SOCK_STREAM,
> + IPPROTO_TCP,
> + p))
> == APR_SUCCESS &&
> apr_sockaddr_info_get(&sa, NULL, APR_INET6, 0, 0, p) ==
> APR_SUCCESS &&
> apr_bind(tmp_sock, sa) == APR_SUCCESS) {
> @@ -253,6 +255,9 @@
> apr_status_t status;
> apr_port_t oldport;
> apr_sockaddr_t *sa;
> +#if APR_HAVE_NETINET_SCTP_H
> + ap_listen_rec *new2;
> +#endif
>
> if (!addr) { /* don't bind to specific interface */
> find_default_family(process->pool);
> @@ -279,10 +284,27 @@
> if (sa) {
> apr_sockaddr_port_get(&oldport, sa);
> if (!strcmp(sa->hostname, addr) && port == oldport) {
> - /* re-use existing record */
why did that comment get removed? I guess protocol can change across
restart? what's up with the clone stuff down below?
> +#if APR_HAVE_NETINET_SCTP_H
check APR feature APR_HAVE_SCTP
> + int protocol;
> + new = *walk;
> + if(new->next){
> + apr_socket_get_protocol(new->next->sd,&protocol);
> + }
> + if(new->next && (protocol == IPPROTO_SCTP )){
> + /* Next one is a clone of this one so
> + * take it too.
> + */
> + *walk = new->next->next;
> + new->next->next = ap_listeners;
> + }else{
> + *walk = new->next;
> + new->next = ap_listeners;
> + }
> +#else
> new = *walk;
> *walk = new->next;
> new->next = ap_listeners;
> +#endif
> ap_listeners = new;
> return NULL;
> }
> @@ -302,12 +324,43 @@
> }
> if ((status = apr_socket_create(&new->sd,
> new->bind_addr->family,
> - SOCK_STREAM, process->pool))
> + SOCK_STREAM,
> + IPPROTO_TCP ,
> + process->pool))
> != APR_SUCCESS) {
> ap_log_perror(APLOG_MARK, APLOG_CRIT, status, process->pool,
> "alloc_listener: failed to get a socket for %s", addr);
> return "Listen setup failed";
> }
> +#if APR_HAVE_NETINET_SCTP_H
> + new2 = apr_palloc(process->pool, sizeof(ap_listen_rec));
> + new2->active = 0;
I'm confused. What directive turns this on?
> diff -u -r1.135 perchild.c
> --- server/mpm/experimental/perchild/perchild.c 11 Oct 2002 15:41:52
> -0000 1.135
> +++ server/mpm/experimental/perchild/perchild.c 14 Oct 2002 11:43:12
> -0000
please, separate patch solely to [EMAIL PROTECTED] for your perchild iov
sendmsg fixes
-----overall comments-----
okay, here is my opinion on how to get this stuff in; others may have
different opinion
A. APR/Apache developers
we have to get our *&%$ together and decide to
a) break existing apps before APR 1.0
b) break existing Apache modules when Apache picks up later APR
or we decide to create apr_socket_create_ex() to use to pass protocol
for APR 1.0
B. you
1) submit cleaned up minimal APR patch to add protocol parm, fix
apr_os_socket_make() to take protocol, create definitions of
APR_PROTO_TCP et al
2) submit related minimal apache patch
(just make call to apr_socket_create() work if we don't go with
apr_socket_create_ex())
C. you, once step B is handled
1) add in remaining SCTP work in APR so that SCTP feature is detected
and represented more cleanly, TCP_NODELAY stuff works, etc.
D. you, once step C is handled
remaining Apache details... including how the feature will be
enabled (but I'm probably being blind and missing how you intend
to control it)
--
Jeff Trawick | [EMAIL PROTECTED]
Born in Roswell... married an alien...
