[PATCH 0/2] Fix or silence "unused result" warnings

[PATCH 0/2] Fix or silence "unused result" warnings

[Austin Clements]

I'm afraid I bikeshedded dme's original patch for this into oblivion
(id:"1324503532-5799-1-git-send-email-dme@dme.org") and we still have
these warnings on the buildbot.  Tomi convinced me that dme was right
and I was wrong, so I'm bringing dme's patch back.

The first patch actually fixes the warning, since the code in show was
wantonly ignoring errors that could have correctness implications.
This write didn't get much attention in the original thread.

The second is a rework of dme's patch to silence the warning for the
remaining writes.  I cleaned up the formatting, simplified the macro
magic a little, and added a comment justifying its use.

[PATCH 1/2] show: Handle read and write errors

[Austin Clements]

For showing a message in raw format, rather than silently succeeding
when a read or a write fails (or, probably, looping if a read fails),
try to print an error message and exit with a non-zero status.

This silences one of the buildbot warnings about unused resuls.  While
my libc lacks the declarations that trigger these warnings, this can
be tested by adding the following to notmuch.h:

__attribute__((warn_unused_result))
size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
---
 notmuch-show.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index d14dac9..c674e25 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -883,7 +883,17 @@ do_show_single (void *ctx,
 
 	while (!feof (file)) {
 	    size = fread (buf, 1, sizeof (buf), file);
-	    (void) fwrite (buf, size, 1, stdout);
+	    if (ferror (file)) {
+		fprintf (stderr, "Error: Read failed from %s\n", filename);
+		fclose (file);
+		return 1;
+	    }
+
+	    if (fwrite (buf, size, 1, stdout) != 1) {
+		fprintf (stderr, "Error: Write failed\n");
+		fclose (file);
+		return 1;
+	    }
 	}
 
 	fclose (file);
-- 
1.7.7.3

[PATCH 2/2] Silence buildbot warnings about unused results

[Austin Clements]

This ignores the results of the two writes in sigint handlers even
harder than before.

While my libc lacks the declarations that trigger these warnings, this
can be tested by adding the following to notmuch.h:

__attribute__((warn_unused_result))
ssize_t write(int fd, const void *buf, size_t count);
---
 compat/compat.h |    8 ++++++++
 notmuch-new.c   |    6 +++++-
 notmuch-tag.c   |    5 ++++-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/compat/compat.h b/compat/compat.h
index 7767fe8..b2e2736 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -46,6 +46,14 @@ getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp);
 char* strcasestr(const char *haystack, const char *needle);
 #endif /* !HAVE_STRCASESTR */
 
+/* Silence gcc warnings about unused results.  These warnings exist
+ * for a reason; any use of this needs to be justified. */
+#ifdef __GNUC__
+#define IGNORE_RESULT(x) ({ __typeof__(x) __z = (x); (void)(__z = __z); })
+#else /* !__GNUC__ */
+#define IGNORE_RESULT(x) x
+#endif /* __GNUC__ */
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/notmuch-new.c b/notmuch-new.c
index 3512de7..a569a54 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -67,7 +67,11 @@ handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
 
-    (void) write(2, msg, sizeof(msg)-1);
+    /* This write is "opportunistic", so it's okay to ignore the
+     * result.  It is not required for correctness, and if it does
+     * fail or produce a short write, we want to get out of the signal
+     * handler as quickly as possible, not retry it. */
+    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 292c5da..1564182 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -26,7 +26,10 @@ static void
 handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
-    (void) write(2, msg, sizeof(msg)-1);
+
+    /* See handle_sigint in notmuch-new.c for the justification for
+     * ignoring write's result. */
+    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
-- 
1.7.7.3

Re: [PATCH 2/2] Silence buildbot warnings about unused results

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

On Wed, 18 Jan 2012 19:40:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> +    /* See handle_sigint in notmuch-new.c for the justification for
> +     * ignoring write's result. */
> +    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));

Just include the comment in both places. Someone will work over the code
in notmuch-new.c and we'll lose it.

Then, +1.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2] Silence buildbot warnings about unused results

[Austin Clements]

Quoth David Edmondson on Jan 19 at  7:23 am:
> On Wed, 18 Jan 2012 19:40:11 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > +    /* See handle_sigint in notmuch-new.c for the justification for
> > +     * ignoring write's result. */
> > +    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
> 
> Just include the comment in both places. Someone will work over the code
> in notmuch-new.c and we'll lose it.

Done.

> Then, +1.

Re: [PATCH 0/2] Fix or silence "unused result" warnings

[Jani Nikula]

On Wed, 18 Jan 2012 19:40:09 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> I'm afraid I bikeshedded dme's original patch for this into oblivion
> (id:"1324503532-5799-1-git-send-email-dme@dme.org") and we still have
> these warnings on the buildbot.  Tomi convinced me that dme was right
> and I was wrong, so I'm bringing dme's patch back.

I'm afraid I contributed to the bikeshedding... My apologies to dme. The
series looks good.

BR,
Jani.

[PATCH v2 1/2] show: Handle read and write errors

[Austin Clements]

For showing a message in raw format, rather than silently succeeding
when a read or a write fails (or, probably, looping if a read fails),
try to print an error message and exit with a non-zero status.

This silences one of the buildbot warnings about unused resuls.  While
my libc lacks the declarations that trigger these warnings, this can
be tested by adding the following to notmuch.h:

__attribute__((warn_unused_result))
size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
---
 notmuch-show.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index d14dac9..c674e25 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -883,7 +883,17 @@ do_show_single (void *ctx,
 
 	while (!feof (file)) {
 	    size = fread (buf, 1, sizeof (buf), file);
-	    (void) fwrite (buf, size, 1, stdout);
+	    if (ferror (file)) {
+		fprintf (stderr, "Error: Read failed from %s\n", filename);
+		fclose (file);
+		return 1;
+	    }
+
+	    if (fwrite (buf, size, 1, stdout) != 1) {
+		fprintf (stderr, "Error: Write failed\n");
+		fclose (file);
+		return 1;
+	    }
 	}
 
 	fclose (file);
-- 
1.7.7.3

[PATCH v2 2/2] Silence buildbot warnings about unused results

[Austin Clements]

This ignores the results of the two writes in sigint handlers even
harder than before.

While my libc lacks the declarations that trigger these warnings, this
can be tested by adding the following to notmuch.h:

__attribute__((warn_unused_result))
ssize_t write(int fd, const void *buf, size_t count);
---
 compat/compat.h |    8 ++++++++
 notmuch-new.c   |    6 +++++-
 notmuch-tag.c   |    5 ++++-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/compat/compat.h b/compat/compat.h
index 7767fe8..b2e2736 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -46,6 +46,14 @@ getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp);
 char* strcasestr(const char *haystack, const char *needle);
 #endif /* !HAVE_STRCASESTR */
 
+/* Silence gcc warnings about unused results.  These warnings exist
+ * for a reason; any use of this needs to be justified. */
+#ifdef __GNUC__
+#define IGNORE_RESULT(x) ({ __typeof__(x) __z = (x); (void)(__z = __z); })
+#else /* !__GNUC__ */
+#define IGNORE_RESULT(x) x
+#endif /* __GNUC__ */
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/notmuch-new.c b/notmuch-new.c
index 3512de7..a569a54 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -67,7 +67,11 @@ handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
 
-    (void) write(2, msg, sizeof(msg)-1);
+    /* This write is "opportunistic", so it's okay to ignore the
+     * result.  It is not required for correctness, and if it does
+     * fail or produce a short write, we want to get out of the signal
+     * handler as quickly as possible, not retry it. */
+    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 292c5da..1564182 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -26,7 +26,10 @@ static void
 handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
-    (void) write(2, msg, sizeof(msg)-1);
+
+    /* See handle_sigint in notmuch-new.c for the justification for
+     * ignoring write's result. */
+    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
-- 
1.7.7.3

[PATCH v3 0/2]

[Austin Clements]

I fail at git.  Here's an actually updated version of this series that
changes the comment in tag's sigint handler.

[PATCH v3 1/2] show: Handle read and write errors

[Austin Clements]

For showing a message in raw format, rather than silently succeeding
when a read or a write fails (or, probably, looping if a read fails),
try to print an error message and exit with a non-zero status.

This silences one of the buildbot warnings about unused resuls.  While
my libc lacks the declarations that trigger these warnings, this can
be tested by adding the following to notmuch.h:

__attribute__((warn_unused_result))
size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
---
 notmuch-show.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index d14dac9..c674e25 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -883,7 +883,17 @@ do_show_single (void *ctx,
 
 	while (!feof (file)) {
 	    size = fread (buf, 1, sizeof (buf), file);
-	    (void) fwrite (buf, size, 1, stdout);
+	    if (ferror (file)) {
+		fprintf (stderr, "Error: Read failed from %s\n", filename);
+		fclose (file);
+		return 1;
+	    }
+
+	    if (fwrite (buf, size, 1, stdout) != 1) {
+		fprintf (stderr, "Error: Write failed\n");
+		fclose (file);
+		return 1;
+	    }
 	}
 
 	fclose (file);
-- 
1.7.7.3

Re: [PATCH v3 1/2] show: Handle read and write errors

[Tomi Ollila]

On Thu, 19 Jan 2012 17:29:18 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> For showing a message in raw format, rather than silently succeeding
> when a read or a write fails (or, probably, looping if a read fails),
> try to print an error message and exit with a non-zero status.
> 
> This silences one of the buildbot warnings about unused resuls.  While
> my libc lacks the declarations that trigger these warnings, this can
> be tested by adding the following to notmuch.h:
> 
> __attribute__((warn_unused_result))
> size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
> ---

+1

Tomi

Re: [PATCH v3 1/2] show: Handle read and write errors

[David Bremner]

On Thu, 19 Jan 2012 17:29:18 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> For showing a message in raw format, rather than silently succeeding
> when a read or a write fails (or, probably, looping if a read fails),
> try to print an error message and exit with a non-zero status.

pushed, 

d

[PATCH v3 2/2] Silence buildbot warnings about unused results

[Austin Clements]

This ignores the results of the two writes in sigint handlers even
harder than before.

While my libc lacks the declarations that trigger these warnings, this
can be tested by adding the following to notmuch.h:

__attribute__((warn_unused_result))
ssize_t write(int fd, const void *buf, size_t count);
---
 compat/compat.h |    8 ++++++++
 notmuch-new.c   |    6 +++++-
 notmuch-tag.c   |    7 ++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/compat/compat.h b/compat/compat.h
index 7767fe8..b2e2736 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -46,6 +46,14 @@ getdelim (char **lineptr, size_t *n, int delimiter, FILE *fp);
 char* strcasestr(const char *haystack, const char *needle);
 #endif /* !HAVE_STRCASESTR */
 
+/* Silence gcc warnings about unused results.  These warnings exist
+ * for a reason; any use of this needs to be justified. */
+#ifdef __GNUC__
+#define IGNORE_RESULT(x) ({ __typeof__(x) __z = (x); (void)(__z = __z); })
+#else /* !__GNUC__ */
+#define IGNORE_RESULT(x) x
+#endif /* __GNUC__ */
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/notmuch-new.c b/notmuch-new.c
index 3512de7..a569a54 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -67,7 +67,11 @@ handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
 
-    (void) write(2, msg, sizeof(msg)-1);
+    /* This write is "opportunistic", so it's okay to ignore the
+     * result.  It is not required for correctness, and if it does
+     * fail or produce a short write, we want to get out of the signal
+     * handler as quickly as possible, not retry it. */
+    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 292c5da..44fd61f 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -26,7 +26,12 @@ static void
 handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
-    (void) write(2, msg, sizeof(msg)-1);
+
+    /* This write is "opportunistic", so it's okay to ignore the
+     * result.  It is not required for correctness, and if it does
+     * fail or produce a short write, we want to get out of the signal
+     * handler as quickly as possible, not retry it. */
+    IGNORE_RESULT (write (2, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
-- 
1.7.7.3

Re: [PATCH v3 2/2] Silence buildbot warnings about unused results

[Tomi Ollila]

On Thu, 19 Jan 2012 17:29:19 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This ignores the results of the two writes in sigint handlers even
> harder than before.
> 
> While my libc lacks the declarations that trigger these warnings, this
> can be tested by adding the following to notmuch.h:
> 
> __attribute__((warn_unused_result))
> ssize_t write(int fd, const void *buf, size_t count);
> ---

+1

Tomi

Re: [PATCH v3 2/2] Silence buildbot warnings about unused results

[David Bremner]

On Thu, 19 Jan 2012 17:29:19 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This ignores the results of the two writes in sigint handlers even
> harder than before.

pushed.

d