719164 - Silence glxtest to get rid of spurious messages from the GL

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 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]]

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]]

err, O_WRONLY instead of O_RDONLY, obviously.

Comment 3

[Mike Hommey [:glandium]]

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)]

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]]

(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)]

#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]]

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)]

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)]

Attachment: redirect all file descriptors to null

Comment 10

[Mike Hommey [:glandium]]

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)]

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]]

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)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ea77fdb9db83

You were right, these includes weren't needed.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea77fdb9db83