[RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[David Edmondson]

---

The mechanism used here works for me in an isolated test case and no
warnings appear when using it as below, but I'm unsure why the
original warning that it is intended to address didn't appear when I
build. Any thoughts?

 compat/compat.h |    6 ++++++
 notmuch-new.c   |    2 +-
 notmuch-show.c  |    2 +-
 notmuch-tag.c   |    2 +-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/compat/compat.h b/compat/compat.h
index 7767fe8..1160301 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -30,6 +30,12 @@
 extern "C" {
 #endif
 
+#ifdef __GNUC__
+#define ignore_result(x) ({ __typeof__(x) z = x; (void) sizeof (z); })
+#else /* !__GNUC__ */
+#define ignore_result(x) x
+#endif /* __GNUC__ */
+
 #if !HAVE_GETLINE
 #include <stdio.h>
 #include <unistd.h>
diff --git a/notmuch-new.c b/notmuch-new.c
index 3512de7..0ac04cc 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -67,7 +67,7 @@ handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
 
-    (void) write(2, msg, sizeof(msg)-1);
+    ignore_result(write(STDERR_FILENO, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 19fb49f..681f778 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -866,7 +866,7 @@ do_show_single (void *ctx,
 
 	while (!feof (file)) {
 	    size = fread (buf, 1, sizeof (buf), file);
-	    (void) fwrite (buf, size, 1, stdout);
+	    ignore_result(fwrite (buf, size, 1, stdout));
 	}
 
 	fclose (file);
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 292c5da..2cbfdc3 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -26,7 +26,7 @@ static void
 handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
-    (void) write(2, msg, sizeof(msg)-1);
+    ignore_result(write(STDERR_FILENO, msg, sizeof(msg)-1));
     interrupted = 1;
 }
 
-- 
1.7.7.3

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[Austin Clements]

I must admit I haven't been following the warnings problem very
closely, but perhaps we shouldn't be ignoring these return codes?

Quoth David Edmondson on Dec 21 at  9:38 pm:
> ---
> 
> The mechanism used here works for me in an isolated test case and no
> warnings appear when using it as below, but I'm unsure why the
> original warning that it is intended to address didn't appear when I
> build. Any thoughts?
> 
>  compat/compat.h |    6 ++++++
>  notmuch-new.c   |    2 +-
>  notmuch-show.c  |    2 +-
>  notmuch-tag.c   |    2 +-
>  4 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/compat/compat.h b/compat/compat.h
> index 7767fe8..1160301 100644
> --- a/compat/compat.h
> +++ b/compat/compat.h
> @@ -30,6 +30,12 @@
>  extern "C" {
>  #endif
>  
> +#ifdef __GNUC__
> +#define ignore_result(x) ({ __typeof__(x) z = x; (void) sizeof (z); })
> +#else /* !__GNUC__ */
> +#define ignore_result(x) x
> +#endif /* __GNUC__ */
> +
>  #if !HAVE_GETLINE
>  #include <stdio.h>
>  #include <unistd.h>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 3512de7..0ac04cc 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -67,7 +67,7 @@ handle_sigint (unused (int sig))
>  {
>      static char msg[] = "Stopping...         \n";
>  
> -    (void) write(2, msg, sizeof(msg)-1);
> +    ignore_result(write(STDERR_FILENO, msg, sizeof(msg)-1));
>      interrupted = 1;
>  }
>  
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 19fb49f..681f778 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -866,7 +866,7 @@ do_show_single (void *ctx,
>  
>  	while (!feof (file)) {
>  	    size = fread (buf, 1, sizeof (buf), file);
> -	    (void) fwrite (buf, size, 1, stdout);
> +	    ignore_result(fwrite (buf, size, 1, stdout));
>  	}
>  
>  	fclose (file);
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 292c5da..2cbfdc3 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -26,7 +26,7 @@ static void
>  handle_sigint (unused (int sig))
>  {
>      static char msg[] = "Stopping...         \n";
> -    (void) write(2, msg, sizeof(msg)-1);
> +    ignore_result(write(STDERR_FILENO, msg, sizeof(msg)-1));
>      interrupted = 1;
>  }
>  

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[David Edmondson]

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

On Thu, 22 Dec 2011 02:03:45 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> I must admit I haven't been following the warnings problem very
> closely, but perhaps we shouldn't be ignoring these return codes?

In general I agree, but what would we do if writing an error message to
stderr fails?

dme.
-- 
David Edmondson, http://dme.org

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

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[Austin Clements]

Quoth David Edmondson on Dec 22 at  7:21 am:
> On Thu, 22 Dec 2011 02:03:45 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > I must admit I haven't been following the warnings problem very
> > closely, but perhaps we shouldn't be ignoring these return codes?
> 
> In general I agree, but what would we do if writing an error message to
> stderr fails?

This was discussed on IRC, but calls to write(2) should never be bare.
I believe it's marked warn_unused_result not because libc is so
concerned with people checking for error returns (otherwise all sorts
of things would be marked warn_unused_result) but because even a
successful write can be a short write.  Hence, not checking the result
is a bug, even if you don't care about errors.

fwrite's a little trickier, since it will only short-write on an
error, so to me it seems perfectly legitimate to ignore the result if
you don't care about errors.

I don't seem to have whatever glibc version the buildbot does that
marks these warn_unused_result, but I can reproduce it by adding

__attribute__((warn_unused_result))
ssize_t write(int fd, const void *buf, size_t count);
__attribute__((warn_unused_result))
size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);

to notmuch-client.h.  Testing with these, if I add any form of result
checking, even if it does nothing in most cases, GCC is quiet.

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[David Edmondson]

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

On Thu, 22 Dec 2011 14:03:05 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > In general I agree, but what would we do if writing an error message to
> > stderr fails?
> 
> This was discussed on IRC, but calls to write(2) should never be bare.
> I believe it's marked warn_unused_result not because libc is so
> concerned with people checking for error returns (otherwise all sorts
> of things would be marked warn_unused_result) but because even a
> successful write can be a short write.  Hence, not checking the result
> is a bug, even if you don't care about errors.

As I said, the principle is sound. What would do in this specific case?

static void
handle_sigint (unused (int sig))
{
    static char msg[] = "Stopping...         \n";

    write(2, msg, sizeof(msg)-1);
    interrupted = 1;
}

Just this?

     if (write(2, msg, sizeof(msg)-1) {
        /* Appease the compiler. */;
     }

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

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[David Edmondson]

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

On Thu, 22 Dec 2011 19:25:59 +0000, David Edmondson <dme@dme.org> wrote:
>      if (write(2, msg, sizeof(msg)-1) {

Sigh.

      if (write(2, msg, sizeof(msg)-1) != sizeof(msg)-1) {

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

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[Austin Clements]

Quoth David Edmondson on Dec 22 at  7:25 pm:
> On Thu, 22 Dec 2011 14:03:05 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > > In general I agree, but what would we do if writing an error message to
> > > stderr fails?
> > 
> > This was discussed on IRC, but calls to write(2) should never be bare.
> > I believe it's marked warn_unused_result not because libc is so
> > concerned with people checking for error returns (otherwise all sorts
> > of things would be marked warn_unused_result) but because even a
> > successful write can be a short write.  Hence, not checking the result
> > is a bug, even if you don't care about errors.
> 
> As I said, the principle is sound. What would do in this specific case?
> 
> static void
> handle_sigint (unused (int sig))
> {
>     static char msg[] = "Stopping...         \n";
> 
>     write(2, msg, sizeof(msg)-1);
>     interrupted = 1;
> }
> 
> Just this?
> 
>      if (write(2, msg, sizeof(msg)-1) {
>         /* Appease the compiler. */;
>      }

Maybe I missed something, but what's wrong with using a standard write
loop (like j4ni suggested on IRC)?  In my mind this isn't about
appeasing the compiler; the compiler is pointing out a real bug.
Patch coming in a moment...

I'm not sure what to do about the specific case of fwrite, though
judging by
  http://sourceware.org/bugzilla/show_bug.cgi?id=11959
I'm not the only person who thinks that fwrite being
warn_unused_result is odd.

[PATCH] Properly handle short writes in sigint handlers

[Austin Clements]

Even if we don't care about errors from write(2), it's still necessary
to handle short writes in order to use write correctly.  Some versions
of glibc even mark write as warn_unused_result because of this, so our
previous usage would trigger compiler warnings.
---
 notmuch-new.c |    5 ++++-
 notmuch-tag.c |    6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 3512de7..fc09bbb 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -66,8 +66,11 @@ static void
 handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
+    const char *pos = msg, *end = msg + sizeof (msg) - 1;
+    ssize_t c = 0;
 
-    (void) write(2, msg, sizeof(msg)-1);
+    for (; pos < end && c >= 0; pos += c)
+	c = write (2, pos, end - pos);
     interrupted = 1;
 }
 
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 292c5da..0d4873d 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -26,7 +26,11 @@ static void
 handle_sigint (unused (int sig))
 {
     static char msg[] = "Stopping...         \n";
-    (void) write(2, msg, sizeof(msg)-1);
+    const char *pos = msg, *end = msg + sizeof (msg) - 1;
+    ssize_t c = 0;
+
+    for (; pos < end && c >= 0; pos += c)
+	c = write (2, pos, end - pos);
     interrupted = 1;
 }
 
-- 
1.7.7.3

Re: [PATCH] Properly handle short writes in sigint handlers

[David Edmondson]

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

Sorry for being slow.

Can you describe the situation in which you expect a write to stderr to
be a short write? (Without error.)

In that situation, what guarantee is there that the loop you've written
will terminate?

We're not talking about safeguarding a users' data here - this is a
short message to indicate that a tool is terminating due to a signal.
I'm concerned that the solution is worse than the problem.

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

Re: [PATCH] Properly handle short writes in sigint handlers

[Tomi Ollila]

On Fri, 23 Dec 2011 08:10:33 +0000, David Edmondson <dme@dme.org> wrote:
> Sorry for being slow.
> 
> Can you describe the situation in which you expect a write to stderr to
> be a short write? (Without error.)

In the following hypothetical case (correct me if I'm wrong :):

* There is 4096 byte buffer in tty driver.
* Stderr is in blocking-mode (the usual case).
* There is already 4090 bytes in that buffer that has not been read.
* One attemtps to write "Stopping...         \n" there (blocks).
* Somehow the system call is interrupted (and SA_RESTART not set)
  -- write() should return 6 bytes written.

But, if the buffer is full already, does the write() system call return
with -1 and EINTR set ?

If there is enough space for all data in that buffer to begin with, 
write() should be atomic.

> In that situation, what guarantee is there that the loop you've written
> will terminate?

If write() keeps returning 0 then it will not terminate (I guess this never
happens). Also, it never terminates if write blocks indefinitely 
(with or without that loop).

> We're not talking about safeguarding a users' data here - this is a
> short message to indicate that a tool is terminating due to a signal.
> I'm concerned that the solution is worse than the problem.

I'm also in favor of "opportunistic" write *in this particular case*

In case that write fails there is most probably more serious things going
on (all resources eaten, hardware problem, etc) and trying to push these
writes forward doesn't help.

Tomi

Re: [PATCH] Properly handle short writes in sigint handlers

[Austin Clements]

Quoth Tomi Ollila on Dec 23 at  2:30 pm:
> On Fri, 23 Dec 2011 08:10:33 +0000, David Edmondson <dme@dme.org> wrote:
> > Sorry for being slow.
> > 
> > Can you describe the situation in which you expect a write to stderr to
> > be a short write? (Without error.)
> 
> In the following hypothetical case (correct me if I'm wrong :):
> 
> * There is 4096 byte buffer in tty driver.
> * Stderr is in blocking-mode (the usual case).
> * There is already 4090 bytes in that buffer that has not been read.
> * One attemtps to write "Stopping...         \n" there (blocks).
> * Somehow the system call is interrupted (and SA_RESTART not set)
>   -- write() should return 6 bytes written.

This is one possibility.  It's also possible it will write no bytes
and fail with EINTR.  Depending on the type of the stderr file
descriptor, it's possible for write to return immediately with 6,
even without a signal interrupting it.

> But, if the buffer is full already, does the write() system call return
> with -1 and EINTR set ?

write only returns EINTR if it was interrupted by a signal before
anything was written.  If the buffer is full already, write will block
(unless it's in non-blocking mode, in which case it will write nothing
and fail with EAGAIN or EWOULDBLOCK).

> If there is enough space for all data in that buffer to begin with, 
> write() should be atomic.
> 
> > In that situation, what guarantee is there that the loop you've written
> > will terminate?
> 
> If write() keeps returning 0 then it will not terminate (I guess this never
> happens). Also, it never terminates if write blocks indefinitely 
> (with or without that loop).

I believe the only way write can return 0 is if you pass it a zero
length.

> > We're not talking about safeguarding a users' data here - this is a
> > short message to indicate that a tool is terminating due to a signal.
> > I'm concerned that the solution is worse than the problem.
> 
> I'm also in favor of "opportunistic" write *in this particular case*
> 
> In case that write fails there is most probably more serious things going
> on (all resources eaten, hardware problem, etc) and trying to push these
> writes forward doesn't help.

This I find more persuasive.  I've been concerned about notmuch doing
strange things (with admittedly minor consequences) under common
circumstances (like transient buffer overflows), but you're right that
more severe circumstances could warrant an opportunistic approach.  Of
course, we're also not depending on the sigint handler for
correctness; if notmuch somehow wedges in it then you're notmuch worse
off than you would be otherwise.

Re: [PATCH] Properly handle short writes in sigint handlers

[Austin Clements]

Quoth David Edmondson on Dec 23 at  8:10 am:
> Sorry for being slow.
> 
> Can you describe the situation in which you expect a write to stderr to
> be a short write? (Without error.)

If the PTY buffer is nearly full because, say, my terminal emulator is
a little behind or my SSH session is slow, I believe POSIX allows this
to be a short write (though Linux appears to treat PTYs like pipes and
will block rather than doing a short write, so it's completely
possible I'm misinterpreting POSIX).

> In that situation, what guarantee is there that the loop you've written
> will terminate?

There isn't, but for the same reason there's also no guarantee that a
single write will terminate.

> We're not talking about safeguarding a users' data here - this is a
> short message to indicate that a tool is terminating due to a signal.
> I'm concerned that the solution is worse than the problem.

As a user I'd be confused to see just part of the "Stopping" message
jammed in the middle of other output, but you're definitely right that
the consequences of this would not extend beyond a little bit of
harmless confusion.

Re: [PATCH] Properly handle short writes in sigint handlers

[Dmitry Kurochkin]

Hi Austin.

On Thu, 22 Dec 2011 15:15:48 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Even if we don't care about errors from write(2), it's still necessary
> to handle short writes in order to use write correctly.  Some versions
> of glibc even mark write as warn_unused_result because of this, so our
> previous usage would trigger compiler warnings.

I think we should put the write loop into a separate function and reuse
it.

Also, does it make sense to add a retry counter to prevent infinite loop
if write keeps returning 0?

Regards,
  Dmitry

> ---
>  notmuch-new.c |    5 ++++-
>  notmuch-tag.c |    6 +++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/notmuch-new.c b/notmuch-new.c
> index 3512de7..fc09bbb 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -66,8 +66,11 @@ static void
>  handle_sigint (unused (int sig))
>  {
>      static char msg[] = "Stopping...         \n";
> +    const char *pos = msg, *end = msg + sizeof (msg) - 1;
> +    ssize_t c = 0;
>  
> -    (void) write(2, msg, sizeof(msg)-1);
> +    for (; pos < end && c >= 0; pos += c)
> +	c = write (2, pos, end - pos);
>      interrupted = 1;
>  }
>  
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 292c5da..0d4873d 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -26,7 +26,11 @@ static void
>  handle_sigint (unused (int sig))
>  {
>      static char msg[] = "Stopping...         \n";
> -    (void) write(2, msg, sizeof(msg)-1);
> +    const char *pos = msg, *end = msg + sizeof (msg) - 1;
> +    ssize_t c = 0;
> +
> +    for (; pos < end && c >= 0; pos += c)
> +	c = write (2, pos, end - pos);
>      interrupted = 1;
>  }
>  
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] Properly handle short writes in sigint handlers

[David Bremner]

On Fri, 23 Dec 2011 23:10:35 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Austin.
> 
> I think we should put the write loop into a separate function and reuse
> it.

I could go either way on this, unless there is somewhere else the code
is actually needed at the moment.

> 
> Also, does it make sense to add a retry counter to prevent infinite loop
> if write keeps returning 0?

I wonder about this too. Is this possibility ignorable Austin?

d

Re: [PATCH] Properly handle short writes in sigint handlers

[Tomi Ollila]

On Tue, 10 Jan 2012 07:13:50 -0400, David Bremner <david@tethera.net> wrote:
> On Fri, 23 Dec 2011 23:10:35 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Hi Austin.
> > 
> > I think we should put the write loop into a separate function and reuse
> > it.
> 
> I could go either way on this, unless there is somewhere else the code
> is actually needed at the moment.
> 
> > 
> > Also, does it make sense to add a retry counter to prevent infinite loop
> > if write keeps returning 0?
> 
> I wonder about this too. Is this possibility ignorable Austin?

In this particular case the consensus was to just ignore the short
write (in rare cases that may happen)... And we could use the
first patch: id:"1324503532-5799-1-git-send-email-dme@dme.org"

For other cases we might try to find a good implementation of 
"writefully()". OpenSSH atomicio.[ch] looks like a good place to start...

... writefully () from that code could look something like:

/* 2-clause license ("Simplified BSD License" or "FreeBSD License") */
size_t writefully(int fd, const void * data, size_t n)
{
	const char *s = (const char *)data;
	size_t pos = 0;
	ssize_t res;

	while (n > pos) {
		res = write(fd, s + pos, n - pos);
		switch (res) {
		case -1:
			if (errno == EINTR)
				continue;
			if (errno == EAGAIN || errno == EWOULDBLOCK) {
                        	struct pollfd pfd;
                                pfd.fd = fd;
                                pfd.events = POLLOUT;
				(void)poll(&pfd, 1, -1);
				continue;
			}
			return 0;
		case 0:
			errno = EPIPE;
			return pos;
		default:
			pos += (size_t)res;
		}
	}
        return pos;
}

> 
> d

Tomi

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[Dmitry Kurochkin]

Hi David.

Perhaps I am missing something here.  But I do not get any warnings when
building with GCC 4.6.2 with -Wall -Werror (-O2 and -O0).  I do not like
adding any complex hacks to make the build warning-free on old GCC
versions.  If this happens on the build bot, we should just update it or
ignore the warnings.

Regards,
  Dmitry

Re: [RFC][PATCH] notmuch: Workaround to allow ignoring non-void function return.

[David Edmondson]

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

On Thu, 22 Dec 2011 13:32:15 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Perhaps I am missing something here.  But I do not get any warnings when
> building with GCC 4.6.2 with -Wall -Werror (-O2 and -O0).  I do not like
> adding any complex hacks to make the build warning-free on old GCC
> versions.  If this happens on the build bot, we should just update it or
> ignore the warnings.

I don't see the warnings on my Debian testing system either.

It would be good to understand what is running on the buildbot.

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