apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@attglobal.net>
Subject Re: [Fwd: A patch for both the APR and HTTPD...]
Date Tue, 15 Oct 2002 21:28:04 GMT
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 :)

> 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...

Mime
View raw message