Last Comment Bug 719164 - Silence glxtest to get rid of spurious messages from the GL
: Silence glxtest to get rid of spurious messages from the GL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-18 11:38 PST by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-02-09 10:16 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
close file descriptors (1.40 KB, patch)
2012-01-18 11:38 PST, Benoit Jacob [:bjacob] (mostly away)
mh+mozilla: review-
Details | Diff | Splinter Review
redirect all file descriptors to null (2.37 KB, patch)
2012-01-31 12:18 PST, Benoit Jacob [:bjacob] (mostly away)
mh+mozilla: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-01-18 11:38:10 PST
Created attachment 589588 [details] [diff] [review]
close file descriptors

Several users have been confused by and have reported to us 'Failed to create drawable' messages from Mesa. It's also a bit confusing to have the glxtest process dump a XPCOM strings leak log.

closing these file descriptors does cause some 'EBADF' errors but afaics it shouldn't matter.

Also in the same patch, a little cleanup of the code determining the filename of the GL library.
Comment 1 Mike Hommey [:glandium] 2012-01-18 11:46:08 PST
Comment on attachment 589588 [details] [diff] [review]
close file descriptors

Review of attachment 589588 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/glxtest.cpp
@@ +112,5 @@
>  {
> +  // close stdout and stderr to prevent spurious output from the GL from appearing in the terminal.
> +  // in particular, Mesa "Failed to create drawable" messages have repeatedly confused people.
> +  close(STDOUT_FILENO);
> +  close(STDERR_FILENO);

Using something like:
  int fd = open("/dev/null", O_RDONLY);
  dup2(fd, STDOUT_FILENO);
  dup2(fd, STDERR_FILENO);
  close(fd);

will avoid the EBADF errors.
Comment 2 Mike Hommey [:glandium] 2012-01-18 11:46:53 PST
err, O_WRONLY instead of O_RDONLY, obviously.
Comment 3 Mike Hommey [:glandium] 2012-01-18 12:01:10 PST
In fact, you should probably do so for any file descriptor < fd, because XPCOM leaks log may be set to use a file and will pollute there.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-01-20 10:33:26 PST
The code proposed in comment 1 and comment 2 does remove the EBADF, but printf then generates ENOTTY errors as explained there:

http://www2.sis.pitt.edu/~ir/KS/Data/Cfaq/q12.24.html

The idea of comment 3 is neat but seems hard to implement in a portable way:

http://stackoverflow.com/questions/899038/getting-the-highest-allocated-file-descriptor

Whatever we do here, being low-maintenance should be the first criterion...
Comment 5 Mike Hommey [:glandium] 2012-01-20 11:08:13 PST
(In reply to Benoit Jacob [:bjacob] from comment #4)
> The code proposed in comment 1 and comment 2 does remove the EBADF, but
> printf then generates ENOTTY errors as explained there:

This doesn't happen here on a small testcase. Does it still happen if you fflush(stdout) and fflush(stderr) before dup2()ing?

> The idea of comment 3 is neat but seems hard to implement in a portable way:
> 
> http://stackoverflow.com/questions/899038/getting-the-highest-allocated-file-
> descriptor

The code doesn't need to be very portable, since it's X11 specific. Moreover, we don't need to care about getting the highest allocated file descriptor. We only care about trying to close the xpcom logging ones, which, in all likeliness, are below "fd" (the value we get from opening /dev/null)
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-01-20 11:14:56 PST
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

int main() {
  int fd = open("/dev/null", O_WRONLY);
  fprintf(stderr, "after open, errno=%d\n", errno);
  fflush(stdout);
  dup2(fd, STDOUT_FILENO);
  fprintf(stderr, "after dup2, errno=%d\n", errno);
  close(fd);
  fprintf(stderr, "after close, errno=%d\n", errno);
  printf("hello\n");
  fprintf(stderr, "after printf, errno=%d\n", errno);
}


Output:

$ g++ a.cpp -o a && ./a
after open, errno=0
after dup2, errno=0
after close, errno=0
after printf, errno=25
Comment 7 Mike Hommey [:glandium] 2012-01-20 11:47:03 PST
Ok, so it wasn't happening on my small test because i was doing a printf before setting stdout to /dev/null, and that doesn't do ENOTTY. Another interesting thing, if just do a main() { printf() } kind of program and run it redirected to /dev/null (./a > /dev/null), it does ENOTTY as well. So it shouldn't matter much.
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-01-20 11:57:01 PST
Is there a reason to believe that EBADF is more of a problem than ENOTTY?

anyway, patch coming.
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-01-31 12:18:04 PST
Created attachment 593178 [details] [diff] [review]
redirect all file descriptors to null
Comment 10 Mike Hommey [:glandium] 2012-02-02 01:30:04 PST
Comment on attachment 593178 [details] [diff] [review]
redirect all file descriptors to null

Review of attachment 593178 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/xre/glxtest.cpp
@@ +61,5 @@
>  
> +#ifdef LINUX
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#endif

What do you need these for? r+ with these removed.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-02-08 10:49:10 PST
They just were mentioned by the open(2) man page, and I seem to remember something like that on Fedora 13 a while ago, but I could be wrong. A version without these includes is now on try:
https://tbpl.mozilla.org/?tree=Try&rev=545f06b4605f
Comment 12 Mike Hommey [:glandium] 2012-02-08 11:00:55 PST
ah, you may need them for the third argument to open, when creating a file, to use macros instead of hardcoding octal values. for normal uses fnctl.h is enough.
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-02-09 05:14:13 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/ea77fdb9db83

You were right, these includes weren't needed.
Comment 14 Ed Morley [:emorley] 2012-02-09 10:16:50 PST
https://hg.mozilla.org/mozilla-central/rev/ea77fdb9db83

Note You need to log in before you can comment on or make changes to this bug.