Re: [PATCH v4 03/12] pkt-line: add packet_flush_gentle()

[Jeff King <peff@peff.net>]

On Wed, Aug 03, 2016 at 06:42:16PM +0200, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> packet_flush() would die in case of a write error even though for some callers
> an error would be acceptable. Add packet_flush_gentle() which writes a pkt-line
> flush packet and returns `0` for success and `1` for failure.

Our normal convention would be "0" for success, "-1" for failure.

I see write_or_whine_pipe(), which you use here, has a bizarre "0 for
failure, 1 for success", but that nobody actually checks it.

I actually think you probably don't want to use write_or_whine_pipe()
here. It does two things:

  1. It writes to stderr unconditionally. But if you are doing a
     "gently" form, then you probably don't want unconditional errors.
     Since the point of not dying is that you could presumably recover
     in some way, or do some other more intelligent action.

     The existing callers of write_or_whine_pipe() are all in the trace
     code. Their use is not "let's handle an error", but "we _would_ die
     except that this is low-priority debugging code that should not
     interrupt the normal flow". So there it at least makes sense to
     unconditionally complain to stderr, but not to die().

     For your series, I don't think that is true (and especially for
     most potential callers of a generic "gently flush the packet"
     function).

  2. It calls check_pipe(), which will turn EPIPE into death-by-SIGPIPE
     (in case you had for some reason ignored SIGPIPE).

     But I think that's the opposite of what you want. You know you're
     writing to a pipe, and I would think EPIPE is the most common
     reason that your writes would fail (i.e., the helper unexpectedly
     died while you were writing to it).

     So you would want to explicitly ignore SIGPIPE while talking to the
     helper, and then handle EPIPE just as any other error.

Thinking about (2), I'd go so far as to say that the trace actually
should just be using:

  if (write_in_full(...) < 0)
	warning("unable to write trace to ...: %s", strerror(errno));

and we should get rid of write_or_whine_pipe entirely.

-Peff