apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Randall Stewart <rand...@stewart.chicago.il.us>
Subject Re: [Fwd: A patch for both the APR and HTTPD...]
Date Wed, 16 Oct 2002 02:22:52 GMT
Jeff Trawick wrote:
> see overall comments at the end
> 
> Randall Stewart <randall@stewart.chicago.il.us> 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
randall@stewart.chicago.il.us 815-342-5222 (cell phone)


Mime
View raw message