You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Randall Stewart <ra...@stewart.chicago.il.us> on 2002/10/14 21:09:53 UTC
[Fwd: A patch for both the APR and HTTPD...]
Attempting again after some
email problems :-0
Thanks
R
--
Randall R. Stewart
randall@stewart.chicago.il.us 815-342-5222 (cell phone)
Re: [Fwd: A patch for both the APR and HTTPD...]
Posted by Randall Stewart <ra...@stewart.chicago.il.us>.
Jeff Trawick wrote:
> see overall comments at the end
>
> Randall Stewart <ra...@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)
Re: [Fwd: A patch for both the APR and HTTPD...]
Posted by Randall Stewart <ra...@stewart.chicago.il.us>.
Jeff Trawick wrote:
> see overall comments at the end
>
> Randall Stewart <ra...@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)
Re: [Fwd: A patch for both the APR and HTTPD...]
Posted by Jeff Trawick <tr...@attglobal.net>.
see overall comments at the end
Randall Stewart <ra...@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 :)
> 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 dev@httpd 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 | trawick@attglobal.net
Born in Roswell... married an alien...
Re: [Fwd: A patch for both the APR and HTTPD...]
Posted by Jeff Trawick <tr...@attglobal.net>.
see overall comments at the end
Randall Stewart <ra...@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 :)
> 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 dev@httpd 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 | trawick@attglobal.net
Born in Roswell... married an alien...