httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Kraemer <Martin.Krae...@mch.sni.de>
Subject Re: [PATCH] TPF port
Date Thu, 15 Apr 1999 08:54:40 GMT
On Fri, Apr 09, 1999 at 07:23:13AM -0600, Joe Moenich wrote:
> we'd like your comments and review also.

> Index: apache/src/include/ap_config.h
> ===================================================================
> RCS file: /apache/apache/src/include/ap_config.h,v
> retrieving revision 1.1.1.1
> diff -u -a -b -r1.1.1.1 ap_config.h
> --- ap_config.h	1999/04/07 13:24:31	1.1.1.1
> +++ ap_config.h	1999/04/07 13:26:54
> @@ -996,6 +1006,11 @@
>  #define strftime(s,max,format,tm)  os_strftime(s,max,format,tm)
>  #endif
>  #include <signal.h>
> +#ifdef TPF
> +#ifdef NSIG
> +#undef NSIG
> +#endif
> +#endif

Why not #if defined(TPF) && defined(NSIG) ? Then you need only one
#endif and make the code clearer.

> @@ -1098,15 +1113,27 @@
> +#ifdef USE_TPF_SELECT
> +#define ap_select   tpf_select
> +#elif defined(SELECT_NEEDS_CAST)
>  #define ap_select(_a, _b, _c, _d, _e)	\
>      select((_a), (int *)(_b), (int *)(_c), (int *)(_d), (_e))
>  #else
>  #define ap_select	select
>  #endif
>  
> +#ifdef USE_TPF_ACCEPT
> +#define ap_accept   tpf_accept
> +#else
> +#define ap_accept   accept
> +#endif
> +
> +#ifdef NEED_SIGNAL_INTERRUPT
> +#define ap_check_signals    tpf_process_signals
> +#else
> +#define ap_check_signals
> +#endif

Uh oh! Bad style! If you want to redefine a function to another
function (or even to nothing) you better use a function-like macro.
Imagine what will happen when NEED_SIGNAL_INTERRUPT is not defined,
and the code in buff.c needs to compile the line...
       ap_check_signals();
What it will see is:
       ();
and I doubt the compiler will make anythink smart out of it. Anyway,
it's always better to use a function-like makro in these cases as it
allows the preprocessor to check for the correct number of arguments
(this only brings you into trouble if you redefine before the actual
function declaration, and that function declaration is K&R style).
So better use something like
> +#define ap_select(_a, _b, _c, _d, _e)  tpf_select(_a, _b, _c, _d, _e)
> +#define ap_check_signals()    tpf_process_signals()
> +#define ap_accept(_fd, _sa, _ln)  tpf_accept(_fd, _sa, _ln)
and similar.

> Index: apache/src/include/scoreboard.h
> ===================================================================
> RCS file: /apache/apache/src/include/scoreboard.h,v
> retrieving revision 1.1.1.1
> diff -u -a -b -r1.1.1.1 scoreboard.h
> --- scoreboard.h	1999/04/07 13:24:31	1.1.1.1
> +++ scoreboard.h	1999/04/07 13:27:02
> @@ -183,6 +183,8 @@
>  } scoreboard;
>  
>  #define SCOREBOARD_SIZE		sizeof(scoreboard)
> +#define SCOREBOARD_NAME		"SCOREBRD"
> +#define SCOREBOARD_FRAMES		SCOREBOARD_SIZE/4095 + 1

This makes it look like SCOREBOARD_NAME and SCOREBOARD_FRAMES were
some values generally important for apache, while in fact they're
only used for TPF. Please add an #ifdef around it (and optionally
add a comment).

> Index: apache/src/main/buff.c
> ===================================================================
> RCS file: /apache/apache/src/main/buff.c,v
> retrieving revision 1.1.1.1
> diff -u -a -b -r1.1.1.1 buff.c
> --- buff.c	1999/04/07 13:24:31	1.1.1.1
> +++ buff.c	1999/04/07 13:27:25
> @@ -263,7 +263,7 @@
> -    tpf_process_signals();
> +    ap_check_signals();

See note above

> Index: apache/src/main/http_main.c
> ===================================================================
> RCS file: /apache/apache/src/main/http_main.c,v
> retrieving revision 1.1.1.1
> diff -u -a -b -r1.1.1.1 http_main.c
> --- http_main.c	1999/04/07 13:24:31	1.1.1.1
> +++ http_main.c	1999/04/07 13:27:46
> @@ -3142,7 +3220,7 @@
>  #endif
>      ap_unblock_alarms();
>  
> -#ifndef WIN32
> +#if !defined(WIN32) & !defined(TPF)

While not exactly incorrect, I'd prefer the logical comparison with
"&&" rather than the bitwise "&"

> @@ -3738,11 +3821,22 @@
>  		case ENETUNREACH:
>  #endif
>                      break;
> -
> +#ifdef TPF
> +		case EINACT:
> +		    ap_log_error(APLOG_MARK, APLOG_EMERG, server_conf,
> +		        "offload device inactive");
> +		    clean_child_exit(1);
> +		    break;

Is there any sense in letting the main server continue at this point?
Would clean_child_exit(APEXIT_CHILDFATAL); be more appropriate?
(Ehm... I don't know what an offload device is; isn't it like a swap
file?)

> @@ -4369,6 +4504,11 @@
>  	    }
>  
>  	    perform_idle_server_maintenance();
> +#ifdef TPF
> +        shutdown_pending = os_check_server(tpf_server_name);
> +        ap_check_signals();
> +        sleep(1);
> +#endif /*TPF */

Isn't the scoreboard maintenance delay handled by....
    tv.tv_sec = SCOREBOARD_MAINTENANCE_INTERVAL / 1000000;
    tv.tv_usec = SCOREBOARD_MAINTENANCE_INTERVAL % 1000000;
    ap_select(0, NULL, NULL, NULL, &tv);
..in http_main? Why is there an additional sleep(1)? And why isn't
it sleep(SCOREBOARD_MAINTENANCE_INTERVAL / 1000000)?

> Index: apache/src/os/tpf/os.c
> ===================================================================
> RCS file: /apache/apache/src/os/tpf/os.c,v
> retrieving revision 1.1.1.1
> diff -u -a -b -r1.1.1.1 os.c
> --- os.c	1999/04/07 13:24:33	1.1.1.1
> +++ os.c	1999/04/07 13:28:13
> +            if (r->method_number == M_PUT)
> +                   ap_bsetflag(r->connection->client, B_ASCII2EBCDIC, 0);

I'm not sure if this is correct here. The decision whether input
conversion has to occur must be based on the Content-Type of the PUT
contents, not on the Content-Type of the returned output. So it must
happen after the (EBCDIC converted) header lines of the PUT body
have been read.

> +#ifndef __PIPE_
Two '_' in front, but only one in the tail?

Cheers,

    Martin
-- 
<Martin.Kraemer@MchP.Siemens.De>      |        Siemens Information and
Phone: +49-89-636-46021               |        Communication  Products
FAX:   +49-89-636-47816               |        81730  Munich,  Germany

Mime
View raw message