| Message ID | 20190303165537.GA23755@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] fetch: avoid calling write_or_die() | expand |
On Sun, Mar 3, 2019 at 11:55 PM Jeff King <peff@peff.net> wrote: > > The write_or_die() function has one quirk that a caller might not > expect: when it sees EPIPE from the write() call, it translates that > into a death by SIGPIPE. This doesn't change the overall behavior (the > program exits either way), but it does potentially confuse test scripts > looking for a non-signal exit code. > > Let's switch away from using write_or_die() in a few code paths, which > will give us more consistent exit codes. It also gives us the > opportunity to write more descriptive error messages, since we have > context that write_or_die() does not. > > Note that this won't do much by itself, since we'd typically be killed > by SIGPIPE before write_or_die() even gets a chance to do its thing. > That will be addressed in the next patch. > > Signed-off-by: Jeff King <peff@peff.net> > --- > fetch-pack.c | 9 ++++++--- > pkt-line.c | 6 ++++-- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/fetch-pack.c b/fetch-pack.c > index 812be15d7e..dca249e9d7 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -191,8 +191,10 @@ static void send_request(struct fetch_pack_args *args, > if (args->stateless_rpc) { > send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX); > packet_flush(fd); > - } else > - write_or_die(fd, buf->buf, buf->len); > + } else { > + if (write_in_full(fd, buf->buf, buf->len) < 0) > + die_errno("unable to write to remote"); maybe _() these strings. > + } > } > > static void insert_one_alternate_object(struct fetch_negotiator *negotiator, > @@ -1163,7 +1165,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > > /* Send request */ > packet_buf_flush(&req_buf); > - write_or_die(fd_out, req_buf.buf, req_buf.len); > + if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0) > + die_errno("unable to write request to remote"); > > strbuf_release(&req_buf); > return ret; > diff --git a/pkt-line.c b/pkt-line.c > index d4b71d3e82..093b2f3976 100644 > --- a/pkt-line.c > +++ b/pkt-line.c > @@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write) > void packet_flush(int fd) > { > packet_trace("0000", 4, 1); > - write_or_die(fd, "0000", 4); > + if (write_in_full(fd, "0000", 4) < 0) > + die_errno("unable to write flush packet"); > } > > void packet_delim(int fd) > { > packet_trace("0001", 4, 1); > - write_or_die(fd, "0001", 4); > + if (write_in_full(fd, "0001", 4) < 0) > + die_errno("unable to write delim packet"); > } > > int packet_flush_gently(int fd) > -- > 2.21.0.675.g01c085a870 >
On Mon, Mar 04, 2019 at 08:42:40PM +0700, Duy Nguyen wrote: > > - } else > > - write_or_die(fd, buf->buf, buf->len); > > + } else { > > + if (write_in_full(fd, buf->buf, buf->len) < 0) > > + die_errno("unable to write to remote"); > > maybe _() these strings. Good thinking. I just sent an update in another part of the thread. -Peff
diff --git a/fetch-pack.c b/fetch-pack.c index 812be15d7e..dca249e9d7 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -191,8 +191,10 @@ static void send_request(struct fetch_pack_args *args, if (args->stateless_rpc) { send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX); packet_flush(fd); - } else - write_or_die(fd, buf->buf, buf->len); + } else { + if (write_in_full(fd, buf->buf, buf->len) < 0) + die_errno("unable to write to remote"); + } } static void insert_one_alternate_object(struct fetch_negotiator *negotiator, @@ -1163,7 +1165,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, /* Send request */ packet_buf_flush(&req_buf); - write_or_die(fd_out, req_buf.buf, req_buf.len); + if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0) + die_errno("unable to write request to remote"); strbuf_release(&req_buf); return ret; diff --git a/pkt-line.c b/pkt-line.c index d4b71d3e82..093b2f3976 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -88,13 +88,15 @@ static void packet_trace(const char *buf, unsigned int len, int write) void packet_flush(int fd) { packet_trace("0000", 4, 1); - write_or_die(fd, "0000", 4); + if (write_in_full(fd, "0000", 4) < 0) + die_errno("unable to write flush packet"); } void packet_delim(int fd) { packet_trace("0001", 4, 1); - write_or_die(fd, "0001", 4); + if (write_in_full(fd, "0001", 4) < 0) + die_errno("unable to write delim packet"); } int packet_flush_gently(int fd)
The write_or_die() function has one quirk that a caller might not expect: when it sees EPIPE from the write() call, it translates that into a death by SIGPIPE. This doesn't change the overall behavior (the program exits either way), but it does potentially confuse test scripts looking for a non-signal exit code. Let's switch away from using write_or_die() in a few code paths, which will give us more consistent exit codes. It also gives us the opportunity to write more descriptive error messages, since we have context that write_or_die() does not. Note that this won't do much by itself, since we'd typically be killed by SIGPIPE before write_or_die() even gets a chance to do its thing. That will be addressed in the next patch. Signed-off-by: Jeff King <peff@peff.net> --- fetch-pack.c | 9 ++++++--- pkt-line.c | 6 ++++-- 2 files changed, 10 insertions(+), 5 deletions(-)