Compiling either 0.6.32 or 0.7.20 on a new Ubuntu 8.10 installation with
gcc 4.3.2 causes errors due to a new compiler warning (when combined
with the use of -Werror in CFLAGS). There is now a compiler warning when
the return value of write() is ignored. This causes errors in a number
of placees in nginx. I have included a log showing the compiler output
as well as a patch file showing the changes I made to avoid these
errors.
On Mon, Nov 10, 2008 at 09:38:39AM -0800, Eric B. wrote:
Compiling either 0.6.32 or 0.7.20 on a new Ubuntu 8.10 installation with gcc 4.3.2 causes errors due to a new compiler warning (when combined with the use of -Werror in CFLAGS). There is now a compiler warning when the return value of write() is ignored. This causes errors in a number of placees in nginx. I have included a log showing the compiler output as well as a patch file showing the changes I made to avoid these errors.
Thank you.
However, there is more elegant way to suppress this warning without
stub variables:
(void) write_fd(...)
Could you test the attached patch ?
Unfortunately the direct cast to void doesn’t suppress the warning. I
tried that first before using the variable assignment (which then
required a cast to void to avoid an unused variable warning!)
Ideally, you should actually check the return value to make sure there
wasn’t an error (-1 returned) or an incomplete write (the number of
bytes written is less than the count argument).
----- Original Message ----
From: Igor S. [email protected]
To: [email protected]
Sent: Monday, November 10, 2008 12:34:37 PM
Subject: Re: Compilation errors on Ubuntu 8.10 Intrepid Ibex
On Mon, Nov 10, 2008 at 09:38:39AM -0800, Eric B. wrote:
Compiling either 0.6.32 or 0.7.20 on a new Ubuntu 8.10 installation with gcc 4.3.2 causes errors due to a new compiler warning (when combined with the use of -Werror in CFLAGS). There is now a compiler warning when the return value of write() is ignored. This causes errors in a number of placees in nginx. I have included a log showing the compiler output as well as a patch file showing the changes I made to avoid these errors.
Thank you.
However, there is more elegant way to suppress this warning without
stub variables:
(void) write_fd(...)
Could you test the attached patch ?
On Tue, Nov 11, 2008 at 12:35:40AM +0300, Maxim D. wrote:
stub variables:
However I don’t think it’s a good idea to use stub variables.
p.s. Actually I think that it’s glibc’s bug. But we probably have
to live with it, at least for some time.
I believe this is gcc bug because I explictly says that I do not need
the function result using (void) case.
Dummy result checking as well as stub variable are security/reliabilty
profanation. The (void) cast should be compact way to do the same.
Whether it is gcc’s bug because cast to void does not suppress the
compiler warning, or it is glibc’s bug because it declares write() with
attribute warn_unused_result, you are unlikely to see a change from
either in the near future. The best course is to take it as an
opportunity to do some paranoid error checking in these cases. In most
cases, there is some better action to be taken when write() fails. In
the worst case, such as trying to write a log entry to a full file
system, it is better to exit() with non-zero status than to silently
continue running. This is why the attribute was added to the declaration
of write(). Most projects do not use -Werror, so they only see added
compiler output instead of termination of the build process. I agree
that it seems like an awfully big change that might adversely affect
many mature software projects. However, there is virtually no case when
it is correct to ignore an exceptional condition in write().
----- Original Message ----
From: Igor S. [email protected]
To: [email protected]
Sent: Monday, November 10, 2008 1:43:54 PM
Subject: Re: Compilation errors on Ubuntu 8.10 Intrepid Ibex
On Tue, Nov 11, 2008 at 12:35:40AM +0300, Maxim D. wrote:
stub variables:
However I don’t think it’s a good idea to use stub variables.
p.s. Actually I think that it’s glibc’s bug. But we probably have
to live with it, at least for some time.
I believe this is gcc bug because I explictly says that I do not need
the function result using (void) case.
Dummy result checking as well as stub variable are security/reliabilty
profanation. The (void) cast should be compact way to do the same.
Hello!
On Tue, Nov 11, 2008 at 12:43:54AM +0300, Igor S. wrote:
On Tue, Nov 11, 2008 at 12:35:40AM +0300, Maxim D. wrote:
[…]
would be better.
Maxim D.
p.s. Actually I think that it’s glibc’s bug. But we probably have
to live with it, at least for some time.I believe this is gcc bug because I explictly says that I do not need
the function result using (void) case.
It can’t be gcc bug - warn_unused_result was explicitly coded to
produce warning unavoidable by (void) cast. I agree that it’s
a misfeature though.
Anyway, there are many handwaving about this at the url I posted
(and in other places as well). I don’t think we want to repeat it
here. 
Dummy result checking as well as stub variable are security/reliabilty
profanation.
With [dummy] result checking we have some good place where we can
write reason why we can’t do anything with the result,
so it looks better for me than stub variable. Of course both
aren’t ideal.
The (void) cast should be compact way to do the same.
It should, but it isn’t. The question now is how to live with it
with minimal impact on decent systems. Probably the best solution
would be just to omit -Werror on such systems.
Maxim D.
Hello!
[… warn_unused_result discussion skipped …]
What about attached patch instead? Actually, there is only 1
place where we really can’t do anything - write() while logging
error. In other places sensible behaviour is possible.
Maxim D.
On Mon, Nov 10, 2008 at 02:27:43PM -0800, Eric B. wrote:
Whether it is gcc’s bug because cast to void does not suppress the compiler warning, or it is glibc’s bug because it declares write() with attribute warn_unused_result, you are unlikely to see a change from either in the near future. The best course is to take it as an opportunity to do some paranoid error checking in these cases. In most cases, there is some better action to be taken when write() fails. In the worst case, such as trying to write a log entry to a full file system, it is better to exit() with non-zero status than to silently continue running. This is why the attribute was added to the declaration of write(). Most projects do not use -Werror, so they only see added compiler output instead of termination of the build process. I agree that it seems like an awfully big change that might adversely affect many mature software projects. However, there is virtually no case when it is correct to ignore an exceptional condition in write().
I believe, that many prefer that server continues to serve site even its
log
filesystem is full. So there is no way how to handle write() error while
logging another error except to ignore it.
Here is the new preliminary patch to test work-around.
Hello!
On Mon, Nov 10, 2008 at 11:34:37PM +0300, Igor S. wrote:
Could you test the attached patch ?
No, warn_unused_result warning can’t be avoided by casting to
void:
$ cat unused.c
int foo() attribute((warn_unused_result));
int
foo()
{
return 1;
}
int
main()
{
(void) foo();
return 0;
}
$ gcc unused.c
unused.c: In function ‘main’:
unused.c:12: warning: ignoring return value of ‘foo’, declared with
attribute warn_unused_result
More details here:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25509
However I don’t think it’s a good idea to use stub variables.
Probably something like
if (ngx_write_fd(...) == NGX_FILE_ERROR) {
/* void */
}
would be better.
Maxim D.
p.s. Actually I think that it’s glibc’s bug. But we probably have
to live with it, at least for some time.
Yes, this works. In fact, after you change it from a macro to a static
function you don’t need a cast to void. So, changing the header file is
sufficient.
I can see the argument for continuing to serve pages even if the log
filesystem is full. I have experience with the opposite case. It all
depends on how critical the logs are for a business. Having been at
Amazon.com in 1996 and 1997 I can tell you that there were times when we
were able to reconstruct some customer transactions using web server
logs after databases were damaged. If it were up to me, I would at least
want this to be a configuration option.
----- Original Message ----
From: Igor S. [email protected]
To: [email protected]
Sent: Monday, November 10, 2008 10:35:36 PM
Subject: Re: Compilation errors on Ubuntu 8.10 Intrepid Ibex
On Mon, Nov 10, 2008 at 02:27:43PM -0800, Eric B. wrote:
Whether it is gcc’s bug because cast to void does not suppress the compiler warning, or it is glibc’s bug because it declares write() with attribute warn_unused_result, you are unlikely to see a change from either in the near future. The best course is to take it as an opportunity to do some paranoid error checking in these cases. In most cases, there is some better action to be taken when write() fails. In the worst case, such as trying to write a log entry to a full file system, it is better to exit() with non-zero status than to silently continue running. This is why the attribute was added to the declaration of write(). Most projects do not use -Werror, so they only see added compiler output instead of termination of the build process. I agree that it seems like an awfully big change that might adversely affect many mature software projects. However, there is virtually no case when it is correct to ignore an exceptional condition in write().
I believe, that many prefer that server continues to serve site even its
log
filesystem is full. So there is no way how to handle write() error while
logging another error except to ignore it.
Here is the new preliminary patch to test work-around.
On Mon, Nov 10, 2008 at 11:25 PM, Eric B. [email protected]
wrote:
I can see the argument for continuing to serve pages even if the log filesystem is full. I have experience with the opposite case. It all depends on how critical the logs are for a business. Having been at Amazon.com in 1996 and 1997 I can tell you that there were times when we were able to reconstruct some customer transactions using web server logs after databases were damaged. If it were up to me, I would at least want this to be a configuration option.
I’d be all for keeping the lights on…
IMHO, if the log gets full, the server shouldn’t be down. My clients
don’t care about logs. They’d rather have their site up. There should
be a separate monitoring and notification system in place for low
space threshholds…