Bug 27623

*** Bug 23453 has been marked as a duplicate of this bug. ***

Review of attachment 34981 [details] [review]:

::: dbus/dbus-spawn.c
@@ +1080,3 @@
         {
           char b;
+          int res = read (sigchld_pipe[READ_END], &b, 1);

Recent gcc will complain that you're not doing anything with res.

In this specific case, it's a pipe-to-self and we don't actually want the data - we just want the side-effect of being woken up from the main loop. Adding "(void) res;", and a comment explaining why we don't care, seems most appropriate.

::: tools/dbus-launch.c
@@ +370,3 @@
+      res = write (1, bus_address, strlen (bus_address) + 1);
+      res = write (1, &bus_pid, sizeof bus_pid);
+      res = write (1, &bus_wid, sizeof bus_wid);

We should check these properly: the purpose of the warning is to get people checking return values so failure to read/write doesn't silently go unreported.

These should probably use do_write() (a local function in dbus-launch) instead of write(), to handle EINTR and die if we fail to write.

This causes a build failure on master:

main.c: In function 'signal_handler':
main.c:94:19: error: ignoring return value of 'write', declared with attribute warn_unused_result [-Werror=unused-result]
main.c:116:19: error: ignoring return value of 'write', declared with attribute warn_unused_result [-Werror=unused-result]
cc1: all warnings being treated as errors

Patches welcome, or configure with CFLAGS=-Wno-error=unused-result.

If you want to fix this (yes please!), I think the reads and writes can be categorized into two classes:

1) gcc has actually spotted a bug

::: tools/dbus-launch.c
@@ +370,3 @@
+      res = write (1, bus_address, strlen (bus_address) + 1);
+      res = write (1, &bus_pid, sizeof bus_pid);
+      res = write (1, &bus_wid, sizeof bus_wid);

These should be checked, for instance via do_write().

2) false positive

I suspect the most correct way to deal with reads/writes that, having audited them, we still don't care about is to use an "if" with an empty body, which is a good place to put the comment justifying why it's OK to ignore the return. Something like this:

    if (read (sigchld_pipe[READ_END], &b, 1) != 1)
      {
        /* We really don't care what the byte was, or whether there
         * even was one: we're only calling read() for its side-effect
         * of draining the pipe.
         *
         * The 'if' is only here to silence -Werror=unused-result.
         */
      }

Scattering (void) casts around seems likely to stop working as gcc gets better at noticing people trying to trick it.

The one in main.c that Arun reported is another false positive: we're only writing to stderr with write() because it's async-signal-safe, and fprintf() isn't. If the write() fails, what are we going to do about it anyway, given that "complain to stderr" is no longer a useful action? I think the only answer we can give is "ignore it and hope for the best".

Appears to have been fixed in master for 1.7.6.