From dev-return-8235-apmail-apr-dev-archive=apr.apache.org@apr.apache.org Wed Oct 16 02:22:56 2002 Return-Path: Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 75798 invoked by uid 500); 16 Oct 2002 02:22:55 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 75787 invoked from network); 16 Oct 2002 02:22:54 -0000 Message-ID: <3DACCD7C.1000702@stewart.chicago.il.us> Date: Tue, 15 Oct 2002 21:22:52 -0500 From: Randall Stewart User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.0rc3) Gecko/20020922 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Jeff Trawick CC: dev@apr.apache.org, dev@httpd.apache.org Subject: Re: [Fwd: A patch for both the APR and HTTPD...] References: <3DAB1681.4010401@stewart.chicago.il.us> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N Jeff Trawick wrote: > see overall comments at the end > > Randall Stewart 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 >>+#endif >>+#include >>+], 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 bombs without > 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 >> #endif >>+#if APR_HAVE_NETINET_SCTP_H >>+#include >>+#include >>+#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 randall@stewart.chicago.il.us 815-342-5222 (cell phone)