The default bug view has changed. See this FAQ.

Silence glxtest to get rid of spurious messages from the GL

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Attachment #589588 - Flags: review?(mh+mozilla)
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.
Attachment #589588 - Flags: review?(mh+mozilla) → review-
err, O_WRONLY instead of O_RDONLY, obviously.
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.
(Assignee)

Comment 4

5 years ago
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...
(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)
(Assignee)

Comment 6

5 years ago
#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
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.
(Assignee)

Comment 8

5 years ago
Is there a reason to believe that EBADF is more of a problem than ENOTTY?

anyway, patch coming.
(Assignee)

Comment 9

5 years ago
Created attachment 593178 [details] [diff] [review]
redirect all file descriptors to null
Attachment #589588 - Attachment is obsolete: true
Attachment #593178 - Flags: review?(mh+mozilla)
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.
Attachment #593178 - Flags: review?(mh+mozilla) → review+
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
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/ea77fdb9db83

You were right, these includes weren't needed.
Assignee: nobody → bjacob
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/ea77fdb9db83
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.