Mailing List Archive: Comments on current patches.

Comments on current patches.

Sep 13, 1995, 2:27 PM

Post #1 of 10 (1159 views)

I just looked at the patches/for_Apache_0.8.13 directory. A few comments:

First off, it looks to me like the 01* patches (in for_Apache_0.8.13) are
intended to fix the problem that the itime patch to mod_log_config which was
offered last time does not compile. However, adding the extra include line
is pointless unless you also toss in the itime modification itself, which is
not included in either of these patches. The right thing here is a *single*
patch (not two, not three) which, when applied to mod_log_config, creates a
new version which includes the itime patch *and* everything else needed to
make it compile cleanly in context. As is, these patches just dink
mod_log_config without doing anything useful.

Second, there are a few things I don't understand about the SCO patch.

*) It increases the size of per-directory configuration vectors by
adding the DYNAMIC_MODULE_LIMIT which came in with last week's
dynamic modules fix. I'm not sure that this is necessary at all;
per-directory configurations are generally created *after* all
dynamically loaded modules are present and initialized. If it
is needed, it should probably be a separate patch.

*) It narrows the 'port' element of the server_rec structure from
int to short. Is this the right thing to do?

*) Finally, wrt the change to the Configure script, if you're going
to delete the -s option to egrep, the egrep should be redirected into
/dev/null; otherwise, when there are syntax errors, we'll get
junk output in addition to the error message.

It would be nice if this could all get sorted out before the vote.

rst

Re: Comments on current patches. [ In reply to ]

ben at gonzo

Sep 14, 1995, 1:18 AM

Post #2 of 10 (1165 views)

Permalink

>
> I just looked at the patches/for_Apache_0.8.13 directory. A few comments:
>
> First off, it looks to me like the 01* patches (in for_Apache_0.8.13) are
> intended to fix the problem that the itime patch to mod_log_config which was
> offered last time does not compile. However, adding the extra include line
> is pointless unless you also toss in the itime modification itself, which is
> not included in either of these patches. The right thing here is a *single*
> patch (not two, not three) which, when applied to mod_log_config, creates a
> new version which includes the itime patch *and* everything else needed to
> make it compile cleanly in context. As is, these patches just dink
> mod_log_config without doing anything useful.
>
> Second, there are a few things I don't understand about the SCO patch.
>
> *) It increases the size of per-directory configuration vectors by
> adding the DYNAMIC_MODULE_LIMIT which came in with last week's
> dynamic modules fix. I'm not sure that this is necessary at all;
> per-directory configurations are generally created *after* all
> dynamically loaded modules are present and initialized. If it
> is needed, it should probably be a separate patch.

Umm. My patch doesn't do that, all it does is add a (void **) to the pcalloc().
The DYNAMIC_MODULE_LIMIT bit is in the original, too. Whether it is right
is another question.

>
> *) It narrows the 'port' element of the server_rec structure from
> int to short. Is this the right thing to do?

Ports are short. If I don't narrow it, I get warnings. It might be more
correct to make it an unsigned short. Excerpt from sys/netinet/in.h:

struct sockaddr_in {
short sin_family;
u_short sin_port; <--- unsigned short, see?
struct in_addr sin_addr;
char sin_zero[8];
};

>
> *) Finally, wrt the change to the Configure script, if you're going
> to delete the -s option to egrep, the egrep should be redirected into
> /dev/null; otherwise, when there are syntax errors, we'll get
> junk output in addition to the error message.

I have to delete -s coz SCO doesn't support it. But, as I read it (from BSDI's
man pages), -s doesn't suppress egrep's output, it suppresses errors caused
by nonexistent or unreadable files. Since egrep is reading from a pipe, -s
is a "no effect" option. Besides, the "junk" output would be the syntax errors.
Rather useful, and not junk at all, IMHO.

>
> It would be nice if this could all get sorted out before the vote.

Sigh. OK, at some point in the nearish, I'll upload 04c..., but not until
people seem to agree on what it should contain.

Cheers,

Ben.

--
Ben Laurie Phone: +44 (181) 994 6435
Freelance Consultant Fax: +44 (181) 994 6472
and Technical Director Email: ben@algroup.co.uk (preferred)
A.L. Digital Ltd, benl@fear.demon.co.uk (backup)
London, England.

[.Note for the paranoid: "fear" as in "Fear and Loathing
in Las Vegas", "demon" as in Demon Internet Services, a
commercial Internet access provider.]

Re: Comments on current patches. [ In reply to ]

randy at zyzzyva

Sep 14, 1995, 5:31 AM

Post #3 of 10 (1161 views)

Permalink

> I just looked at the patches/for_Apache_0.8.13 directory. A few comments:
>
> First off, it looks to me like the 01* patches (in for_Apache_0.8.13) are
> intended to fix the problem that the itime patch to mod_log_config which was
> offered last time does not compile. However, adding the extra include line
> is pointless unless you also toss in the itime modification itself, which is
> not included in either of these patches. The right thing here is a *single*
> patch (not two, not three) which, when applied to mod_log_config, creates a
> new version which includes the itime patch *and* everything else needed to
> make it compile cleanly in context. As is, these patches just dink
> mod_log_config without doing anything useful.

I just uploaded 01a_log_config_integer_time.0.8.13.patch.

It includes the few changes to support integer time format, and
adds the needed #include that was missing from last weeks patch.

There was one other change in the previous 01_ patch that I did not
include that Aram had added to his patch. I suspect that this is a
change to remove some warnings for the Linux du jour? They should
probably be in another patch since they don't apply to the integer
time changes, that I am aware of.

*** conf.h.orig Tue Sep 12 11:30:23 1995
--- conf.h Tue Sep 12 11:30:29 1995
***************
*** 181,189 ****
#undef NO_KILLPG
#undef NO_SETSID
#undef NEED_STRDUP
- #define FD_SET __FD_SET
- #define FD_ZERO __FD_ZERO
- #define FD_ISSET __FD_ISSET
#define JMP_BUF sigjmp_buf
#define FCNTL_SERIALIZED_ACCEPT

Re: Comments on current patches. [ In reply to ]

awm at qosina

Sep 14, 1995, 6:34 AM

Post #4 of 10 (1159 views)

Permalink

At 07:31 AM 9/14/95 -0500, you wrote:
>> I just looked at the patches/for_Apache_0.8.13 directory. A few comments:
>>
>> First off, it looks to me like the 01* patches (in for_Apache_0.8.13) are
>> intended to fix the problem that the itime patch to mod_log_config which was
>> offered last time does not compile. However, adding the extra include line
>> is pointless unless you also toss in the itime modification itself, which is
>> not included in either of these patches. The right thing here is a *single*
>> patch (not two, not three) which, when applied to mod_log_config, creates a
>> new version which includes the itime patch *and* everything else needed to
>> make it compile cleanly in context. As is, these patches just dink
>> mod_log_config without doing anything useful.
>
>I just uploaded 01a_log_config_integer_time.0.8.13.patch.

Actually I'm going to withdraw the 01 patch, rst is completely right about
this falling into a "make a look better" than a patch fix. So when ever the
original patch is resumbited, I'll re do these patches. It would be nice to
be able to clean the code up a bit, but not neccessary at the moment.

>
>It includes the few changes to support integer time format, and
>adds the needed #include that was missing from last weeks patch.
>
>There was one other change in the previous 01_ patch that I did not
>include that Aram had added to his patch. I suspect that this is a
>change to remove some warnings for the Linux du jour? They should
>probably be in another patch since they don't apply to the integer
>time changes, that I am aware of.

Actually the problem with the below came about with the include of
<sys/time.h>. The below variables are defined in <sys/time.h> that's
why they were in the same patch.

<Aram>
>
>
>*** conf.h.orig Tue Sep 12 11:30:23 1995
>--- conf.h Tue Sep 12 11:30:29 1995
>***************
>*** 181,189 ****
> #undef NO_KILLPG
> #undef NO_SETSID
> #undef NEED_STRDUP
>- #define FD_SET __FD_SET
>- #define FD_ZERO __FD_ZERO
>- #define FD_ISSET __FD_ISSET
> #define JMP_BUF sigjmp_buf
> #define FCNTL_SERIALIZED_ACCEPT
>
>
>
>
>
--
Aram W. Mirzadeh, MIS Manager, Qosina Corporation
http://www.qosina.com/~awm/, awm@qosina.com
Apache httpd server team http://www.apache.org

Re: Comments on current patches. [ In reply to ]

rst at ai

Sep 14, 1995, 7:03 AM

Post #5 of 10 (1159 views)

Permalink

My patch doesn't do that, all it does is add a (void **) to the pcalloc().
The DYNAMIC_MODULE_LIMIT bit is in the original, too.

Your patch is adding the DYNAMIC_MODULE_LIMIT stuff in places where
it wasn't there before, and probably isn't needed. That's what I
was asking about.

As to egrep, here's the description of the -s option under SunOS:

-s Work silently, that is, display nothing except error
messages. This is useful for checking the error
status.

Note that *all* output is suppressed.

If there are syntax errors, then the Config script is at pains to
print them out, *after* labeling them as what they are. The effect
of deleting the -s option is to make the output in this case
unnecessarily messy and confusing --- a Bad Thing generally, but
especially where error messages are concerned.

rst

Re: Comments on current patches. [ In reply to ]

ben at gonzo

Sep 14, 1995, 7:27 AM

Post #6 of 10 (1163 views)

Permalink

>
> >> Second, there are a few things I don't understand about the SCO patch.
> >>
> >> *) It increases the size of per-directory configuration vectors by
> >> adding the DYNAMIC_MODULE_LIMIT which came in with last week's
> >> dynamic modules fix. I'm not sure that this is necessary at all;
> >> per-directory configurations are generally created *after* all
> >> dynamically loaded modules are present and initialized. If it
> >> is needed, it should probably be a separate patch.
> >
> >Umm. My patch doesn't do that, all it does is add a (void **) to the
> >pcalloc(). The DYNAMIC_MODULE_LIMIT bit is in the original, too. Whether it
> >is right is another question.
>
> What original? It's not in 0.8.13.

It is in the version I downloaded from the FTP site. Did you get your
0.8.13 by some other route?

> >>
> >> *) It narrows the 'port' element of the server_rec structure from
> >> int to short. Is this the right thing to do?
> >
> >Ports are short. If I don't narrow it, I get warnings. It might be more
> >correct to make it an unsigned short. Excerpt from sys/netinet/in.h:
> >...
>
> The warning being harmless, of course.

Warnings are never harmless. At least, when they happen on my screen. I
have to go and look at the code to see if it is broken. It is our policy
to always compile clean, including warnings.

>
> >> *) Finally, wrt the change to the Configure script, if you're going
> >> to delete the -s option to egrep, the egrep should be redirected into
> >> /dev/null; otherwise, when there are syntax errors, we'll get
> >> junk output in addition to the error message.
> >
> >I have to delete -s coz SCO doesn't support it. But, as I read it (from BSDI's
> >man pages), -s doesn't suppress egrep's output, it suppresses errors caused
> >by nonexistent or unreadable files.
>
> This is incorrect. -s (on some systems at least) supresses egrep output
> except for error messages. (The opposite of what you suggest.

A script which does not know which system it is on has no business using
options that do one thing on one system and another on another. Or even cause
it to completely fail on some (e.g. SCO when -s is used).

> >Since egrep is reading from a pipe, -s is a "no effect" option.
>
> Not true (see above).

True on some systems (e.g. BSDI) (see above).

>
> >Besides, the "junk" output would be the syntax errors. Rather useful, and not
> >junk at all, IMHO.
>
> Wrong; have another look at the script. If this egrep fails then the script
> itself generates messages about the syntax errors. So output from this
> egrep really is "junk".

Ooops, missed that bit. Anyway, the portable way to fix this is to delete the
-s and append a >/dev/null, yes?

> > > It would be nice if this could all get sorted out before the vote.
> >
> >Sigh. OK, at some point in the nearish, I'll upload 04c..., but not until
> >people seem to agree on what it should contain.
>
> Why does the patch define SIGURG as SIGUSR1? Wouldn't it be better to disable
> SIGURG completely? (By making the use of it in http_main.c #ifdef SIGURG'ed)

Simply because SIGUSR1 is the nearest equivalent of SIGURG. It at least
saves anyone from having to figure that out again. I did it before I realised
an ioctl() was needed anyway, but then though it would be best to leave it in
case someone implements the ioctl(). What I am curious to know is when people
are expecting SIGURG to occur, and on what systems (since there seems to be
some variation between systems).

>
> The patch contains a C++ style comment; this needs to be removed.

My apologies. It shall be removed.

>
> David.

--
Ben Laurie Phone: +44 (181) 994 6435
Freelance Consultant Fax: +44 (181) 994 6472
and Technical Director Email: ben@algroup.co.uk (preferred)
A.L. Digital Ltd, benl@fear.demon.co.uk (backup)
London, England.

[.Note for the paranoid: "fear" as in "Fear and Loathing
in Las Vegas", "demon" as in Demon Internet Services, a
commercial Internet access provider.]

Re: Comments on current patches. [ In reply to ]

rst at ai

Sep 14, 1995, 8:37 AM

Post #7 of 10 (1162 views)

Permalink

It is in the version I downloaded from the FTP site. Did you get your
0.8.13 by some other route?

Arrrgh. I rechecked the patch again, and you are correct --- your patch
does *not* add the DYNAMIC_MODULE_LIMIT stuff anywhere that it wasn't
present already. Sorry for the confusion. The egrep business still
deserves attention though.

(This is particularly true if there are systems which have a -s option
which does not suppress normal output, in which case redirecting stdout
and stderr to /dev/null explicitly really is the only way to go).

Sorry again about the confusion.

rst

Re: Comments on current patches. [ In reply to ]

ben at gonzo

Sep 14, 1995, 8:41 AM

Post #8 of 10 (1159 views)

Permalink

>
> My patch doesn't do that, all it does is add a (void **) to the pcalloc().
> The DYNAMIC_MODULE_LIMIT bit is in the original, too.
>
> Your patch is adding the DYNAMIC_MODULE_LIMIT stuff in places where
> it wasn't there before, and probably isn't needed. That's what I
> was asking about.

No it isn't. Please name the places where you think it is adding it, because
I cannot for the life of me see any. And, until a moment from now, I have
never typed DYNAMIC_MODULE_LIMIT. There is a patch which includes
_two_ patches in one lump, one of which has a DYNAMIC_MODULE_LIMIT and one of
which doesn't, which, I suppose, if glanced at, might _look_ like an extra
DYNAMIC_MODULE_LIMIT, but actually isn't.

>
> As to egrep, here's the description of the -s option under SunOS:
>
> -s Work silently, that is, display nothing except error
> messages. This is useful for checking the error
> status.
>
> Note that *all* output is suppressed.
>
> If there are syntax errors, then the Config script is at pains to
> print them out, *after* labeling them as what they are. The effect
> of deleting the -s option is to make the output in this case
> unnecessarily messy and confusing --- a Bad Thing generally, but
> especially where error messages are concerned.

As I suggested earlier, the solution seems to be to delete the -s and add
a >/dev/null, since -s is completely broken on SCO and does the wrong thing
on BSDI (and, no doubt, others).

>
> rst
>
>

--
Ben Laurie Phone: +44 (181) 994 6435
Freelance Consultant Fax: +44 (181) 994 6472
and Technical Director Email: ben@algroup.co.uk (preferred)
A.L. Digital Ltd, benl@fear.demon.co.uk (backup)
London, England.

[.Note for the paranoid: "fear" as in "Fear and Loathing
in Las Vegas", "demon" as in Demon Internet Services, a
commercial Internet access provider.]

Re: Comments on current patches. [ In reply to ]

drtr at ast

Sep 14, 1995, 10:11 AM

Post #9 of 10 (1158 views)

Permalink

>> Second, there are a few things I don't understand about the SCO patch.
>>
>> *) It increases the size of per-directory configuration vectors by
>> adding the DYNAMIC_MODULE_LIMIT which came in with last week's
>> dynamic modules fix. I'm not sure that this is necessary at all;
>> per-directory configurations are generally created *after* all
>> dynamically loaded modules are present and initialized. If it
>> is needed, it should probably be a separate patch.
>
>Umm. My patch doesn't do that, all it does is add a (void **) to the
>pcalloc(). The DYNAMIC_MODULE_LIMIT bit is in the original, too. Whether it
>is right is another question.

What original? It's not in 0.8.13.

>>
>> *) It narrows the 'port' element of the server_rec structure from
>> int to short. Is this the right thing to do?
>
>Ports are short. If I don't narrow it, I get warnings. It might be more
>correct to make it an unsigned short. Excerpt from sys/netinet/in.h:
>...

The warning being harmless, of course.

>> *) Finally, wrt the change to the Configure script, if you're going
>> to delete the -s option to egrep, the egrep should be redirected into
>> /dev/null; otherwise, when there are syntax errors, we'll get
>> junk output in addition to the error message.
>
>I have to delete -s coz SCO doesn't support it. But, as I read it (from BSDI's
>man pages), -s doesn't suppress egrep's output, it suppresses errors caused
>by nonexistent or unreadable files.

This is incorrect. -s (on some systems at least) supresses egrep output
except for error messages. (The opposite of what you suggest.

>Since egrep is reading from a pipe, -s is a "no effect" option.

Not true (see above).

>Besides, the "junk" output would be the syntax errors. Rather useful, and not
>junk at all, IMHO.

Wrong; have another look at the script. If this egrep fails then the script
itself generates messages about the syntax errors. So output from this
egrep really is "junk".

> > It would be nice if this could all get sorted out before the vote.
>
>Sigh. OK, at some point in the nearish, I'll upload 04c..., but not until
>people seem to agree on what it should contain.

Why does the patch define SIGURG as SIGUSR1? Wouldn't it be better to disable
SIGURG completely? (By making the use of it in http_main.c #ifdef SIGURG'ed)

The patch contains a C++ style comment; this needs to be removed.

David.

Re: Comments on current patches. [ In reply to ]

sanders at bsdi

Sep 14, 1995, 12:35 PM

Post #10 of 10 (1161 views)

Permalink

Robert S. Thau writes:
> As to egrep, here's the description of the -s option under SunOS:
>
> -s Work silently, that is, display nothing except error
> messages. This is useful for checking the error
> status.
>
> Note that *all* output is suppressed.

POSIX.2 (IEEE 1003.2-1992) specifies that -s "Suppress error messages
about nonexistent or unreadable files." The -q option suppresses
normal output.