httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From moen...@us.ibm.com
Subject Re: [PATCH] TPF port
Date Thu, 15 Apr 1999 14:19:39 GMT


Martin.   Thanks for the comments.

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

I'll clean this up.

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

I definitely see your point.  I'll get this corrected also.

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

Will do.

>> 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 "&"

Don't know how I managed to let this one slip by.  Good catch,  thanks.

>> @@ -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?)

You're right, there is no reason to let the main server continue.  An
offload device is actually a box attached to TPF performing most of the
TCPIP services.  It's generally something like an OS2 or CISCO box.

>> @@ -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)?

In TPF we have several methods of starting and stopping Apache.  In one
model,  Apache must check with the daemon to determine if it should
shutdown, hence the "os_check_server".  In a second model, the daemon will
send a kill to the Apache parent.  When started from the commandline,
Apache continues until the user manually kills the process.

The sleep(1) call is necessary to give control temporarily back to the
system.  TPF has a built-in mechanism to prevent runaway processes seeing
it isn't usual to have long running processes on the system (typical length
of a process is 2-3 seconds).  For example, if someone coded an infinite
loop on TPF, after a specified time, the system will terminate the process
and dump with a control 10.  As the while(restart_pending ...) loop is
infinite until a shutdown request is received, we give control back to the
system with the sleep function, preventing Apache from abending.

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

This is being reinvestigated.  As we (Siemens and IBM) are the only EBCDIC
boxes, the programmer is contacting you outside the forum about this.

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

Yep,  that's how it's defined in the header file.


Joe Moenich
moenich@us.ibm.com
303 773-5483
tie-line 656-7487



Mime
View raw message