Jeff Trawick wrote:
> 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 :)
>
well maybe and maybe not.. this is the tricky thing.. SCTP will
seamlessly replace TCP... with the only thing that
doing a setsockopt(...TCP_NODELAY) will fail... other than
that it would all work.. its just you would be on the SCTP side
listening for say.. a ftp connection ;-0
>
>>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.
>
sctp.h generally uses u_int8_t/u_int16_t/u_int32_t.. but it may
be able to be a lot simpler... this was my first venture into the
world of configure.in so it might be able to be done much easier.. all
I wanted was to validate that sctp.h existed... Sorry for my ignorance..
I just did a quick read of the online doc's on autoconf and came
up with the first thing I coudl.. your APR_FLAGS_HEADERS() if it
checks the existance of netinet/sctp.h is MUCH MUCH better...
>
>>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
>
>
I thought that this would be a good thing but I was attempting to do
the minimum amount of change :-0
This would definetly be a more generic solution..
>>--- 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)
>
Is there a document on style.. sorry about this.. I typcially use
a BSDish format.. for kame... and even then I always seem to blow it
;-0
>
>>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
>
I would find it very very hard to believe you would have one
without the other.. but I guess a check to make sure both
existed would not be a bad thing...
>
>>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)
>
so you tab not space... hmm I bet my emacs profile
writes out spaces... a pointer to a proper .emacs from
someone would be much appreciated... and then I can let
emacs do the proper thing :>
>
>>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)
>
Yeah that is one of my bad habit that itojun rides me about too...
>
>>+ 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
>
ok.. that makes sense...
>
>>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 '>'?
>
hmm.. it should not have been there.. I bet it was a cut and
paste error .. sigh...no compiler to make sure I did it right ....
>
>>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)
>
yep. Which file would you want this APR_PROTO_TCP defined in?
>
>>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?
>
Hmm.. I did not realize I had wiped a comment.. I had to transfer
my changes manually from one file to another and I must have cut
it by mistake...sigh..
>
>>+#if APR_HAVE_NETINET_SCTP_H
>
>
> check APR feature APR_HAVE_SCTP
>
yep
>
>>+ 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?
>
In the SCTP case.. it would either open or it would not... if it
failed to open you would get a warning out and that it did not
get an sctp socket and that is it..
The idea is that SCTP listens in parrallel to tcp whenever there
is a listener.. I did not see a directive for TCP so I assumed
that since you asked to listen on a port... that would mean
on the available protocols....
In the TCP case you go critical if you don't get it.. in the SCTP case
it is not critical... just a whine..
>
>
>>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 dev@httpd for your perchild iov
> sendmsg fixes
>
Ok.. I will go back and re-work the style issues and build seperate
patches...
> -----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
>
actually the addtion of apr_socket_create_ex actually sounds like a
good idea.. and then just inside apr_socket_create() force it to
TCP or UDP in the protocol field... at least it seems like a great
idea to me :>
> 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
>
sounds good...
> 2) submit related minimal apache patch
> (just make call to apr_socket_create() work if we don't go with
> apr_socket_create_ex())
>
ok.. I will await what others think on the apr_socket_create_ex..
but it sounds like an elegant way to go...
> 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)
>
Not sure that it needs control... you either can listen on a
SCTP socket or not... If SCTP is present it will listen.. if not
it won't... I did not even see this as a issue....unless you are
thinking of having something like a
Listen 0.0.0.0:80/tcp
Listen 0.0.0.0:80/sctp
I did not think that was really needed... since a simple
Listen 0.0.0.0:80
Can have both SCTP and TCP listen to port 80 without a problem...
Somehow I am missing the issue..hmm...
--
Randall R. Stewart
[EMAIL PROTECTED] 815-342-5222 (cell phone)
