You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Marc Slemko <ma...@znep.com> on 1998/07/09 08:43:57 UTC
win32 os_stat question
first, all these ap_asserts in the win32 code are dumb. They don't tell
you anything useful, don't give useful errors, and are not just on obscure
code paths but are things that I have to fight with just trying to make it
run.
Second, I don't understand the first assert in os_stat.
ap_assert(szPath[1] == ':' || szPath[1] == '/'); // we are dealing with either UNC or a drive
if(szPath[0] == '/') {
[...]
What the heck is that? We say it can't be something, then we check
to see if it is something? This makes my config with "/apache"
for paths die here. Changing the szPath[1] to szPath[0] makes
it work for me, but I don't know the intent here.
Also, a trailing '/' on a directory name (ie. /apache/, don't
know about c:/apache/) makes the server refuse to start.
Then comes the fact that "Directory /" blows up in
sub_canonical_filename. Wee problem given that the default
config file includes that.
While compared to it taking three hours for me to change
my dialup networking to use a different modem and connect
to a different server (not because I didn't know how, just
because I had to reboot a zillion times and deal with the
configuration programs crashing), taking me half an hour
to make Apache work isn't bad, it really isn't quite decent
for release...
Re: win32 os_stat question
Posted by Alexei Kosut <ak...@leland.Stanford.EDU>.
On Fri, 10 Jul 1998, Ben Laurie wrote:
[...]
> It is the general style of Apache to not assert at all. But the point in
> this case is that you shouldn't call os_stat with a not-fully-qualified
> path. The error checking should go elsewhere, IMO.
An assert should only be used to verify things we know to be true. Or
expect to be true. When it is possible - and legal - for the user to
configure the server in a certain way that we don't like, asserts are not
a valid way to handle the situation.
I much perfer
if (szPath[1] != ':' && szPath[1] != '/') {
fprintf(stderr, "httpd: Illegal path name %s", szPath);
exit(1);
}
to
ap_assert(szPath[1] == ':' || szPath[1] == '/');
The latter is meaningless to users.
[...]
> > Why is /apache incorrect? The docs say it should work, it works fine for
> > me if I remove the assert. It was only broken when you added stuff to
> > deal with UNCs.
>
> a) it is incorrect because it doesn't specify which drive you are on. If
> you are happy to have it choose from various drives at random, by all
> means deem it to be correct.
>
> b) So what if it was broken when I added UNCs?
For the reasons Marc specified. I know it worked fine last summer.
Although I am confused - os_canonical_filename() is supposed to fix this
(that's the reason for the GetFullPathName() call - one of its features is
that it adds drive letters to paths without them), so why is os_stat
getting such a filename?
[...]
> > Yea, but I have this odd idea that the config files as shipped should
> > work.
>
> I never use the shipped config files. They are completely stupid:
Regardless, Marc is correct. If Apache, as shipped, does not work with the
config files, as shipped, then one of the two must be fixed. I consider
this sufficient cause to hold up a release.
P.S. The docs/config file comments are wrong, in any case, since AFAIK,
they still don't mention that all filenames should be in all-lowercase.
-- Alexei Kosut <ak...@stanford.edu> <http://www.stanford.edu/~akosut/>
Stanford University, Class of 2001 * Apache <http://www.apache.org> *
Re: win32 os_stat question
Posted by Ben Laurie <be...@algroup.co.uk>.
I don't really see the point of all these histrionics. os_stat() is
clearly not the place for meaningful error messages. Now I look at it,
the assert() is there because I wasn't sure I had analysed cases where
the path wasn't absolute, but I agree it causes problems with seemingly
OK configurations (I say seemingly because the assertion in the
documentation that the drive will be implictly the same as Apache's
executable's seems to me to be entirely without foundation). If you are
prepared to debug the weird cases that arise when the documentation's
assertion is false, then remove the ap_assert(). If not, then track down
what causes the assertion to fire and fix it.
The assert() probably shouldn't be there anyway, because stat() does,
and hence os_stat() should, deal with relative paths, so I'd suggest
that latter course followed by the former :-)
Chers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
WE'RE RECRUITING! http://www.aldigital.co.uk/recruit/
Re: win32 os_stat question
Posted by Marc Slemko <ma...@worldgate.com>.
On Fri, 10 Jul 1998, Ben Laurie wrote:
> Marc Slemko wrote:
> >
> > On Thu, 9 Jul 1998, Ben Laurie wrote:
> >
> > Regarding asserts, the problem is that they are being used as an excuse to
> > avoid proper error checking and handling. It is not the general style of
> > the Apache code to assert everywhere and just die if it isn't happy with a
> > condition. It is completely inexcusable in a server, especially a
> > threaded one, to have the whole thing exit just because an unexpected
> > condition was reached without proper error checking.
>
> It is the general style of Apache to not assert at all. But the point in
> this case is that you shouldn't call os_stat with a not-fully-qualified
> path. The error checking should go elsewhere, IMO.
Erm... no.
>
> > Because of the way asserts are used without logging anything useful and in
> > replacement of proper error checking, it took me a good half hour to
> > configure Apache so it would run on my box, starting from the current code
> > tree. I ran into numerous cases where all I would get is a useless assert
> > that doesn't tell me anything, then I would have to go into the code and
> > hack in debugging messages to say what is going on.
>
> What could os_stat log that is useful?
The path, plus some meaningful text about what the heck the issue is. At
the very least, it could actually return an error when there is an error
so higher levels can deal with it.
> > I think we should add something to the style guide about assert()s not
> > being a replacement for proper error handling.
>
> The assert in this case is intended to indicate to the programmer that
> he has not done proper error handling. os_stat() is not in a position to
> do proper error handling.
And neither is the programmer, because they can't handle errors from
os_stat.
If you call a Unix stat(), then if the path is busted you get an error and
can deal with it. It makes no sense to gratuitously change the semantics
for Win32 and require a whole lot of extra code be added just for the
Win32 case.
[...]
> > Why is /apache incorrect? The docs say it should work, it works fine for
> > me if I remove the assert. It was only broken when you added stuff to
> > deal with UNCs.
>
> a) it is incorrect because it doesn't specify which drive you are on. If
> you are happy to have it choose from various drives at random, by all
> means deem it to be correct.
>
> b) So what if it was broken when I added UNCs?
Breaking something that is documented and used (although it may not have
worked 100% correctly) just because you don't care about it isn't a good
thing.
>
> >
> > eg.
> >
> > * The directives that accept filenames as arguments now must use
> > Windows filenames instead of Unix ones. However, because Apache
> > uses Unix-style names internally, you must use forward slashes,
> > not backslashes. Drive letters can be used; if omitted, the drive
> > with the Apache executable will be assumed.
>
> If you want to make that true, then the code that read "/apache" should
> prepend the drive letter.
>
> Or are you saying that os_stat (and every other call that takes a
> filename) should prepend the executable's drive letter?
>
> Just coz someone takes it into their head to write something on a doc
> page doesn't mean that's what the code does, and it certainly doesn't
> mean it is suddenly my responsibility to make it come true.
No, but if you change it from working for people, even if it isn't
completly right, to not working then you should be sure that is reflected
properly.
>
> > > > Also, a trailing '/' on a directory name (ie. /apache/, don't
> > > > know about c:/apache/) makes the server refuse to start.
> > > >
> > > > Then comes the fact that "Directory /" blows up in
> > > > sub_canonical_filename. Wee problem given that the default
> > > > config file includes that.
> > > >
> > > > While compared to it taking three hours for me to change
> > > > my dialup networking to use a different modem and connect
> > > > to a different server (not because I didn't know how, just
> > > > because I had to reboot a zillion times and deal with the
> > > > configuration programs crashing), taking me half an hour
> > > > to make Apache work isn't bad, it really isn't quite decent
> > > > for release...
> > >
> > > Can you say "beta"?
> >
> > Yea, but I have this odd idea that the config files as shipped should
> > work.
>
> I never use the shipped config files. They are completely stupid:
That's great, but it doesn't mean that the shipped config files shouldn't
work.
Re: win32 os_stat question
Posted by Ben Laurie <be...@algroup.co.uk>.
Marc Slemko wrote:
>
> On Thu, 9 Jul 1998, Ben Laurie wrote:
>
> Regarding asserts, the problem is that they are being used as an excuse to
> avoid proper error checking and handling. It is not the general style of
> the Apache code to assert everywhere and just die if it isn't happy with a
> condition. It is completely inexcusable in a server, especially a
> threaded one, to have the whole thing exit just because an unexpected
> condition was reached without proper error checking.
It is the general style of Apache to not assert at all. But the point in
this case is that you shouldn't call os_stat with a not-fully-qualified
path. The error checking should go elsewhere, IMO.
> Because of the way asserts are used without logging anything useful and in
> replacement of proper error checking, it took me a good half hour to
> configure Apache so it would run on my box, starting from the current code
> tree. I ran into numerous cases where all I would get is a useless assert
> that doesn't tell me anything, then I would have to go into the code and
> hack in debugging messages to say what is going on.
What could os_stat log that is useful?
> I think we should add something to the style guide about assert()s not
> being a replacement for proper error handling.
The assert in this case is intended to indicate to the programmer that
he has not done proper error handling. os_stat() is not in a position to
do proper error handling.
> > > Second, I don't understand the first assert in os_stat.
> > >
> > > ap_assert(szPath[1] == ':' || szPath[1] == '/'); // we are dealing with either UNC or a drive
> > > if(szPath[0] == '/') {
> > > [...]
> > >
> > > What the heck is that? We say it can't be something, then we check
> > > to see if it is something? This makes my config with "/apache"
> > > for paths die here. Changing the szPath[1] to szPath[0] makes
> > > it work for me, but I don't know the intent here.
> >
> > I don't understand what you think we say it can't be but then check for,
> > but anyway, the intent is that you should supply an absolute path. That
> > means it either starts with x:/ or //somemachine. /apache is incorrect:
> > you've missed out the drive.
>
> Why is /apache incorrect? The docs say it should work, it works fine for
> me if I remove the assert. It was only broken when you added stuff to
> deal with UNCs.
a) it is incorrect because it doesn't specify which drive you are on. If
you are happy to have it choose from various drives at random, by all
means deem it to be correct.
b) So what if it was broken when I added UNCs?
>
> eg.
>
> * The directives that accept filenames as arguments now must use
> Windows filenames instead of Unix ones. However, because Apache
> uses Unix-style names internally, you must use forward slashes,
> not backslashes. Drive letters can be used; if omitted, the drive
> with the Apache executable will be assumed.
If you want to make that true, then the code that read "/apache" should
prepend the drive letter.
Or are you saying that os_stat (and every other call that takes a
filename) should prepend the executable's drive letter?
Just coz someone takes it into their head to write something on a doc
page doesn't mean that's what the code does, and it certainly doesn't
mean it is suddenly my responsibility to make it come true.
> > > Also, a trailing '/' on a directory name (ie. /apache/, don't
> > > know about c:/apache/) makes the server refuse to start.
> > >
> > > Then comes the fact that "Directory /" blows up in
> > > sub_canonical_filename. Wee problem given that the default
> > > config file includes that.
> > >
> > > While compared to it taking three hours for me to change
> > > my dialup networking to use a different modem and connect
> > > to a different server (not because I didn't know how, just
> > > because I had to reboot a zillion times and deal with the
> > > configuration programs crashing), taking me half an hour
> > > to make Apache work isn't bad, it really isn't quite decent
> > > for release...
> >
> > Can you say "beta"?
>
> Yea, but I have this odd idea that the config files as shipped should
> work.
I never use the shipped config files. They are completely stupid:
a) they use three files even though we advise against it.
b) people use them but with a small number of changes making it
difficult to figure out what they've done that is non-default.
So, if you want them to work, you need to fix them.
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
WE'RE RECRUITING! http://www.aldigital.co.uk/recruit/
Re: win32 os_stat question
Posted by Marc Slemko <ma...@worldgate.com>.
On Thu, 9 Jul 1998, Ben Laurie wrote:
Regarding asserts, the problem is that they are being used as an excuse to
avoid proper error checking and handling. It is not the general style of
the Apache code to assert everywhere and just die if it isn't happy with a
condition. It is completely inexcusable in a server, especially a
threaded one, to have the whole thing exit just because an unexpected
condition was reached without proper error checking.
Because of the way asserts are used without logging anything useful and in
replacement of proper error checking, it took me a good half hour to
configure Apache so it would run on my box, starting from the current code
tree. I ran into numerous cases where all I would get is a useless assert
that doesn't tell me anything, then I would have to go into the code and
hack in debugging messages to say what is going on.
I think we should add something to the style guide about assert()s not
being a replacement for proper error handling.
> > Second, I don't understand the first assert in os_stat.
> >
> > ap_assert(szPath[1] == ':' || szPath[1] == '/'); // we are dealing with either UNC or a drive
> > if(szPath[0] == '/') {
> > [...]
> >
> > What the heck is that? We say it can't be something, then we check
> > to see if it is something? This makes my config with "/apache"
> > for paths die here. Changing the szPath[1] to szPath[0] makes
> > it work for me, but I don't know the intent here.
>
> I don't understand what you think we say it can't be but then check for,
> but anyway, the intent is that you should supply an absolute path. That
> means it either starts with x:/ or //somemachine. /apache is incorrect:
> you've missed out the drive.
Why is /apache incorrect? The docs say it should work, it works fine for
me if I remove the assert. It was only broken when you added stuff to
deal with UNCs.
eg.
* The directives that accept filenames as arguments now must use
Windows filenames instead of Unix ones. However, because Apache
uses Unix-style names internally, you must use forward slashes,
not backslashes. Drive letters can be used; if omitted, the drive
with the Apache executable will be assumed.
> > Also, a trailing '/' on a directory name (ie. /apache/, don't
> > know about c:/apache/) makes the server refuse to start.
> >
> > Then comes the fact that "Directory /" blows up in
> > sub_canonical_filename. Wee problem given that the default
> > config file includes that.
> >
> > While compared to it taking three hours for me to change
> > my dialup networking to use a different modem and connect
> > to a different server (not because I didn't know how, just
> > because I had to reboot a zillion times and deal with the
> > configuration programs crashing), taking me half an hour
> > to make Apache work isn't bad, it really isn't quite decent
> > for release...
>
> Can you say "beta"?
Yea, but I have this odd idea that the config files as shipped should
work.
Re: win32 os_stat question
Posted by Ben Laurie <be...@algroup.co.uk>.
Marc Slemko wrote:
>
> first, all these ap_asserts in the win32 code are dumb. They don't tell
> you anything useful, don't give useful errors, and are not just on obscure
> code paths but are things that I have to fight with just trying to make it
> run.
So replace them with useful error messages. I'd rather have an
ap_assert() than blindly plough on until the sky falls on my head, but a
proper error is even better.
> Second, I don't understand the first assert in os_stat.
>
> ap_assert(szPath[1] == ':' || szPath[1] == '/'); // we are dealing with either UNC or a drive
> if(szPath[0] == '/') {
> [...]
>
> What the heck is that? We say it can't be something, then we check
> to see if it is something? This makes my config with "/apache"
> for paths die here. Changing the szPath[1] to szPath[0] makes
> it work for me, but I don't know the intent here.
I don't understand what you think we say it can't be but then check for,
but anyway, the intent is that you should supply an absolute path. That
means it either starts with x:/ or //somemachine. /apache is incorrect:
you've missed out the drive.
> Also, a trailing '/' on a directory name (ie. /apache/, don't
> know about c:/apache/) makes the server refuse to start.
>
> Then comes the fact that "Directory /" blows up in
> sub_canonical_filename. Wee problem given that the default
> config file includes that.
>
> While compared to it taking three hours for me to change
> my dialup networking to use a different modem and connect
> to a different server (not because I didn't know how, just
> because I had to reboot a zillion times and deal with the
> configuration programs crashing), taking me half an hour
> to make Apache work isn't bad, it really isn't quite decent
> for release...
Can you say "beta"?
Cheers,
Ben.
--
Ben Laurie |Phone: +44 (181) 735 0686| Apache Group member
Freelance Consultant |Fax: +44 (181) 735 0689|http://www.apache.org/
and Technical Director|Email: ben@algroup.co.uk |
A.L. Digital Ltd, |Apache-SSL author http://www.apache-ssl.org/
London, England. |"Apache: TDG" http://www.ora.com/catalog/apache/
WE'RE RECRUITING! http://www.aldigital.co.uk/recruit/