| Message ID | 20201027065250.GB3005508@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sideband: diagnose more incoming packet anomalies | expand |
On Tue, Oct 27, 2020 at 02:52:50AM -0400, Jeff King wrote: > - we'll report an eof with a more detailed "protocol error: eof while > reading sideband packet", rather than the generic "the remote end > hung up unexpectedly" I wasn't sure if "eof" was too jargon-y. This is probably a message normal users are likely to see if the network drops out on them. So I'm open to rewording suggestions. Or even dropping the eof bits entirely. They do give us better flushing behavior: > - when we see an eof, we'll flush the sideband scratch buffer, which > may provide some hints from the remote about why they hung up > (though note we already flush on newlines, so it's likely that most > such messages already made it through) But I suspect that's uncommon in practice. > t/t0070-fundamental.sh | 12 ++++++++++++ I added this to t0070 since that's where your earlier test went. It does feel like this isn't quite as "fundamental" as some of the other things in that file, though. > +test_expect_success 'missing sideband designator is reported' ' > + printf 0004 >input && > + test-tool pkt-line receive-sideband <input 2>err && > + test_i18ngrep "missing sideband" err > +' I found it much more straight-forward to just printf the sample input, rather than writing C code to create it, as your test did. It keeps the input and expectation together, and makes them easier to extend or change. I wonder if it's worth doing this: t/helper/test-pkt-line.c | 16 ---------------- t/t0070-fundamental.sh | 7 ++++++- t/test-lib-functions.sh | 12 +++++++++++- 3 files changed, 17 insertions(+), 18 deletions(-) diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c index 5e638f0b97..0615833bec 100644 --- a/t/helper/test-pkt-line.c +++ b/t/helper/test-pkt-line.c @@ -84,20 +84,6 @@ static void unpack_sideband(void) } } -static int send_split_sideband(void) -{ - const char *part1 = "Hello,"; - const char *primary = "\001primary: regular output\n"; - const char *part2 = " world!\n"; - - send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX); - packet_write(1, primary, strlen(primary)); - send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX); - packet_response_end(1); - - return 0; -} - static int receive_sideband(void) { return recv_sideband("sideband", 0, 1); @@ -114,8 +100,6 @@ int cmd__pkt_line(int argc, const char **argv) unpack(); else if (!strcmp(argv[1], "unpack-sideband")) unpack_sideband(); - else if (!strcmp(argv[1], "send-split-sideband")) - send_split_sideband(); else if (!strcmp(argv[1], "receive-sideband")) receive_sideband(); else diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 936030a5ef..f9fda8d610 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -35,7 +35,12 @@ test_expect_success 'check for a bug in the regex routines' ' ' test_expect_success 'incomplete sideband messages are reassembled' ' - test-tool pkt-line send-split-sideband >split-sideband && + { + packetize -2 "Hello," && + packetize -1 "primary: regular output" && + packetize -2 " world!" && + printf 0000 + } >split-sideband && test-tool pkt-line receive-sideband <split-sideband 2>err && grep "Hello, world" err ' diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 8d59b90348..c90ea61747 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -1427,10 +1427,20 @@ nongit () { # given on stdin, and that empty input becomes an empty packet, not a flush # packet (for that you can just print 0000 yourself). packetize() { + band= + bandlen=0 + case "$1" in + -[123]) + band="\\${1#-}" + bandlen=1 + shift + ;; + esac + if test $# -gt 0 then packet="$*" - printf '%04x%s' "$((4 + ${#packet}))" "$packet" + printf "%04x$band%s" "$((4 + $bandlen + ${#packet}))" "$packet" else perl -e ' my $packet = do { local $/; <STDIN> }; Teaching packetize() about bands isn't strictly necessary (we could printf the band designators ourselves and pipe it into packetize(); that would also add the missing newlines, though those do not matter for this test). -Peff
On Tue, Oct 27, 2020 at 02:52:50AM -0400, Jeff King wrote: > On Mon, Oct 26, 2020 at 04:09:13PM +0000, Johannes Schindelin via GitGitGadget wrote: > > > Changes since v2: > > > > * Dropped patch 3/3 because it was only intended to be defensive > > programming, but turned out to be too hard without layering violations. > > It is a bit tricky, but here's a working replacement. And one other small cleanup, worth doing with or without my patch. -- >8 -- Subject: [PATCH] test-pkt-line: drop colon from sideband identity We pass "sideband: " as our identity for errors to recv_sideband(). But it already adds the trailing colon and space. This doesn't invalidate any tests, but it looks funny when you examine the test output. Signed-off-by: Jeff King <peff@peff.net> --- t/helper/test-pkt-line.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c index 0bf20642be..5e638f0b97 100644 --- a/t/helper/test-pkt-line.c +++ b/t/helper/test-pkt-line.c @@ -100,7 +100,7 @@ static int send_split_sideband(void) static int receive_sideband(void) { - return recv_sideband("sideband: ", 0, 1); + return recv_sideband("sideband", 0, 1); } int cmd__pkt_line(int argc, const char **argv)
Jeff King <peff@peff.net> writes: > On Tue, Oct 27, 2020 at 02:52:50AM -0400, Jeff King wrote: > >> - we'll report an eof with a more detailed "protocol error: eof while >> reading sideband packet", rather than the generic "the remote end >> hung up unexpectedly" > > I wasn't sure if "eof" was too jargon-y. This is probably a message > normal users are likely to see if the network drops out on them. We were expecting to see something, and instead of seeing that specific thing we were waiting, we saw the end-of-file? That sounds like "the other side hung up unexpectedly" to me. Does it bother you for being "too generic" because it does not say what we were expecting to see? "unexpected disconnect while waiting for a sideband packet"?
Jeff King <peff@peff.net> writes: > And one other small cleanup, worth doing with or without my patch. > > -- >8 -- > Subject: [PATCH] test-pkt-line: drop colon from sideband identity > > We pass "sideband: " as our identity for errors to recv_sideband(). But > it already adds the trailing colon and space. This doesn't invalidate > any tests, but it looks funny when you examine the test output. > > Signed-off-by: Jeff King <peff@peff.net> > --- Makes sense and it is a no-brainer to understand why it is the right thing to do. I'll queue it directly on top of two patches from Dscho. Thanks. > t/helper/test-pkt-line.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c > index 0bf20642be..5e638f0b97 100644 > --- a/t/helper/test-pkt-line.c > +++ b/t/helper/test-pkt-line.c > @@ -100,7 +100,7 @@ static int send_split_sideband(void) > > static int receive_sideband(void) > { > - return recv_sideband("sideband: ", 0, 1); > + return recv_sideband("sideband", 0, 1); > } > > int cmd__pkt_line(int argc, const char **argv)
On Tue, Oct 27, 2020 at 11:56:46AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Tue, Oct 27, 2020 at 02:52:50AM -0400, Jeff King wrote: > > > >> - we'll report an eof with a more detailed "protocol error: eof while > >> reading sideband packet", rather than the generic "the remote end > >> hung up unexpectedly" > > > > I wasn't sure if "eof" was too jargon-y. This is probably a message > > normal users are likely to see if the network drops out on them. > > We were expecting to see something, and instead of seeing that > specific thing we were waiting, we saw the end-of-file? > > That sounds like "the other side hung up unexpectedly" to me. Does > it bother you for being "too generic" because it does not say what > we were expecting to see? "unexpected disconnect while waiting for > a sideband packet"? Yes, saying what we were expecting is what seems like an improvement to me. I tried coming up with something that said "hung up" but the wording ended up awkward. Your "unexpected disconnect while waiting for a sideband packet" is exactly what I was trying for. :) Does this patch seem otherwise worth doing? An alternate patch would be to keep the behavior the same and just clarify the code a bit. Something like: -- >8 -- Subject: [PATCH] demultiplex_sideband(): clarify corner cases The size checks in demultiplex_sideband() are a bit subtle and confusing: - we consider a zero-length packet ("0004") to be a flush packet, even though it's not really one. This is perhaps wrong, but it should never happen in our protocol, and we err on the side of history and leniency. We'll leave a comment indicating that we expect this case. - likewise we consider any flush-like packet (e.g., "0001" delim) to be a flush. I didn't confirm whether this is necessary for normal protocol usage. It may be for the everything-over-sideband mode introduced by 0bbc0bc574 ({fetch,upload}-pack: sideband v2 fetch response, 2019-01-16). Likewise let's leave a comment. - we check for "len < 1" to see if there's no sideband designator. That's confusing, because we already covered the "len == 0" case. What is interesting is the "len < 0" case. But that's not a missing sideband designator, but rather an error or EOF from the pkt-line code. This should never happen, though, because our callers instruct pkt-line to die() on EOF anyway. So let's make it more obvious that we're looking for a negative value here, and consider it a BUG() in the caller to pass us garbage. Signed-off-by: Jeff King <peff@peff.net> --- sideband.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/sideband.c b/sideband.c index 0a60662fa6..6ba1925614 100644 --- a/sideband.c +++ b/sideband.c @@ -130,17 +130,18 @@ int demultiplex_sideband(const char *me, char *buf, int len, suffix = DUMB_SUFFIX; } + if (len < 0) + BUG("error/eof packet passed to demultiplex_sideband"); + if (len == 0) { + /* + * we treat all flush-like packets (flush, delim, etc) and even + * empty data packets as a flush + */ *sideband_type = SIDEBAND_FLUSH; goto cleanup; } - if (len < 1) { - strbuf_addf(scratch, - "%s%s: protocol error: no band designator", - scratch->len ? "\n" : "", me); - *sideband_type = SIDEBAND_PROTOCOL_ERROR; - goto cleanup; - } + band = buf[0] & 0xff; buf[len] = '\0'; len--;
Jeff King <peff@peff.net> writes: > Does this patch seem otherwise worth doing? Yeah. FWIW, I do not find the "dependency" thing disturbing. sideband is an extension of the pkt-line mechansim, so it is natural that it depends on pkt-line. I'd also be happy if enums, structures and calls defined in both headers are made available by just including one of them (e.g. retire sideband.h, perhaps). > An alternate patch would be to keep the behavior the same and just > clarify the code a bit. Something like: This also looks OK to me from readability's point of view, but it does not as much help the end user who is puzzled as the real thing, I am afraid. Thanks. > -- >8 -- > Subject: [PATCH] demultiplex_sideband(): clarify corner cases > > The size checks in demultiplex_sideband() are a bit subtle and > confusing: > > - we consider a zero-length packet ("0004") to be a flush packet, even > though it's not really one. This is perhaps wrong, but it should > never happen in our protocol, and we err on the side of history and > leniency. We'll leave a comment indicating that we expect this case. > > - likewise we consider any flush-like packet (e.g., "0001" delim) to > be a flush. I didn't confirm whether this is necessary for normal > protocol usage. It may be for the everything-over-sideband mode > introduced by 0bbc0bc574 ({fetch,upload}-pack: sideband v2 fetch > response, 2019-01-16). Likewise let's leave a comment. > > - we check for "len < 1" to see if there's no sideband designator. > That's confusing, because we already covered the "len == 0" case. > What is interesting is the "len < 0" case. But that's not a missing > sideband designator, but rather an error or EOF from the pkt-line > code. This should never happen, though, because our callers instruct > pkt-line to die() on EOF anyway. So let's make it more obvious > that we're looking for a negative value here, and consider it a > BUG() in the caller to pass us garbage. > > Signed-off-by: Jeff King <peff@peff.net> > --- > sideband.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/sideband.c b/sideband.c > index 0a60662fa6..6ba1925614 100644 > --- a/sideband.c > +++ b/sideband.c > @@ -130,17 +130,18 @@ int demultiplex_sideband(const char *me, char *buf, int len, > suffix = DUMB_SUFFIX; > } > > + if (len < 0) > + BUG("error/eof packet passed to demultiplex_sideband"); > + > if (len == 0) { > + /* > + * we treat all flush-like packets (flush, delim, etc) and even > + * empty data packets as a flush > + */ > *sideband_type = SIDEBAND_FLUSH; > goto cleanup; > } > - if (len < 1) { > - strbuf_addf(scratch, > - "%s%s: protocol error: no band designator", > - scratch->len ? "\n" : "", me); > - *sideband_type = SIDEBAND_PROTOCOL_ERROR; > - goto cleanup; > - } > + > band = buf[0] & 0xff; > buf[len] = '\0'; > len--;
On Tue, Oct 27, 2020 at 02:38:12PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Does this patch seem otherwise worth doing? > > Yeah. OK, here it is, then with that one error message reworded. I dropped "protocol error" from that one message, as well. One, because it was getting quite long. And two, it's probably _not_ a protocol error, but rather a network drop. So just describing what we see on the client side is the best strategy. > FWIW, I do not find the "dependency" thing disturbing. sideband is > an extension of the pkt-line mechansim, so it is natural that it > depends on pkt-line. I'd also be happy if enums, structures and > calls defined in both headers are made available by just including > one of them (e.g. retire sideband.h, perhaps). I agree with that. Given how easy it was to work around with an int, I'm not inclined to spend time going further at this point, but it's something to keep in mind. > > An alternate patch would be to keep the behavior the same and just > > clarify the code a bit. Something like: > > This also looks OK to me from readability's point of view, but it > does not as much help the end user who is puzzled as the real thing, > I am afraid. I suspect these cases are rare enough that nobody minds either way. But I do think the behavior-changing patch is making us more likely to do the right thing. The downside is the risk of some unintended regression, but I feel pretty good about it. -- >8 -- Subject: [PATCH] sideband: diagnose more sideband anomalies In demultiplex_sideband(), there are two oddities when we check an incoming packet: - if it has zero length, then we assume it's a flush packet. This means we fail to notice the difference between a real flush and a true zero-length packet that's missing its sideband designator. It's not a huge problem in practice because we'd never send a zero-length data packet (even our keepalives are otherwise-empty sideband-1 packets). But it would be nice to detect and report the error, since it's likely to cause other confusion (we think the other side flushed, but they do not). - we try to detect packets missing their designator by checking for "if (len < 1)". But this will never trigger for "len == 0"; we've already detected that and left the function before then. It _could_ detect a negative "len" parameter. But in that case, the error message is wrong. The issue is not "no sideband" but rather "eof while reading the packet". However, this can't actually be triggered in practice, because neither of the two callers uses pkt_read's GENTLE_ON_EOF flag. Which means they'd die with "the remote end hung up unexpectedly" before we even get here. So this truly is dead code. We can improve these cases by passing in a pkt-line status to the demultiplexer, and by having recv_sideband() use GENTLE_ON_EOF. This gives us two improvements: - we can now reliably detect flush packets, and will report a normal packet missing its sideband designator as an error - we'll report an eof with a more detailed "protocol error: eof while reading sideband packet", rather than the generic "the remote end hung up unexpectedly" - when we see an eof, we'll flush the sideband scratch buffer, which may provide some hints from the remote about why they hung up (though note we already flush on newlines, so it's likely that most such messages already made it through) In some sense this patch goes against fbd76cd450 (sideband: reverse its dependency on pkt-line, 2019-01-16), which caused the sideband code not to depend on the pkt-line code. But that commit was really just trying to deal with the circular header dependency. The two modules are conceptually interlinked, and it was just trying to keep things compiling. And indeed, there's a sticking point in this patch: because pkt-line.h includes sideband.h, we can't add the reverse include we need for the sideband code to have an "enum packet_read_status" parameter. Nor can we forward declare it, because you can't forward declare an enum in C. However, C does guarantee that enums fit in an int, so we can just use that type. One alternative would be for the callers to check themselves that they got something sane from the pkt-line code. But besides duplicating logic, this gets quite tricky. Any error condition requires flushing the sideband #2 scratch buffer, which only demultiplex_sideband() knows how to do. Signed-off-by: Jeff King <peff@peff.net> --- pkt-line.c | 14 ++++++++------ sideband.c | 29 ++++++++++++++++++++++------- sideband.h | 6 +++++- t/t0070-fundamental.sh | 12 ++++++++++++ 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/pkt-line.c b/pkt-line.c index 657a702927..d633005ef7 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -461,9 +461,11 @@ int recv_sideband(const char *me, int in_stream, int out) enum sideband_type sideband_type; while (1) { - len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, - 0); - if (!demultiplex_sideband(me, buf, len, 0, &scratch, + int status = packet_read_with_status(in_stream, NULL, NULL, + buf, LARGE_PACKET_MAX, + &len, + PACKET_READ_GENTLE_ON_EOF); + if (!demultiplex_sideband(me, status, buf, len, 0, &scratch, &sideband_type)) continue; switch (sideband_type) { @@ -520,9 +522,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader) reader->options); if (!reader->use_sideband) break; - if (demultiplex_sideband(reader->me, reader->buffer, - reader->pktlen, 1, &scratch, - &sideband_type)) + if (demultiplex_sideband(reader->me, reader->status, + reader->buffer, reader->pktlen, 1, + &scratch, &sideband_type)) break; } diff --git a/sideband.c b/sideband.c index a5405b9aaa..6f9e026732 100644 --- a/sideband.c +++ b/sideband.c @@ -3,6 +3,7 @@ #include "config.h" #include "sideband.h" #include "help.h" +#include "pkt-line.h" struct keyword_entry { /* @@ -114,7 +115,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) #define ANSI_SUFFIX "\033[K" #define DUMB_SUFFIX " " -int demultiplex_sideband(const char *me, char *buf, int len, +int demultiplex_sideband(const char *me, int status, + char *buf, int len, int die_on_error, struct strbuf *scratch, enum sideband_type *sideband_type) @@ -130,17 +132,30 @@ int demultiplex_sideband(const char *me, char *buf, int len, suffix = DUMB_SUFFIX; } - if (len == 0) { - *sideband_type = SIDEBAND_FLUSH; - goto cleanup; - } - if (len < 1) { + if (status == PACKET_READ_EOF) { strbuf_addf(scratch, - "%s%s: protocol error: no band designator", + "%s%s: unexpected disconnect while reading sideband packet", scratch->len ? "\n" : "", me); *sideband_type = SIDEBAND_PROTOCOL_ERROR; goto cleanup; } + + if (len < 0) + BUG("negative length on non-eof packet read"); + + if (len == 0) { + if (status == PACKET_READ_NORMAL) { + strbuf_addf(scratch, + "%s%s: protocol error: missing sideband designator", + scratch->len ? "\n" : "", me); + *sideband_type = SIDEBAND_PROTOCOL_ERROR; + } else { + /* covers flush, delim, etc */ + *sideband_type = SIDEBAND_FLUSH; + } + goto cleanup; + } + band = buf[0] & 0xff; buf[len] = '\0'; len--; diff --git a/sideband.h b/sideband.h index 227740a58e..5a25331be5 100644 --- a/sideband.h +++ b/sideband.h @@ -18,8 +18,12 @@ enum sideband_type { * * scratch must be a struct strbuf allocated by the caller. It is used to store * progress messages split across multiple packets. + * + * The "status" parameter is a pkt-line response as returned by + * packet_read_with_status() (e.g., PACKET_READ_NORMAL). */ -int demultiplex_sideband(const char *me, char *buf, int len, +int demultiplex_sideband(const char *me, int status, + char *buf, int len, int die_on_error, struct strbuf *scratch, enum sideband_type *sideband_type); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 357201640a..8d59905ef0 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -40,4 +40,16 @@ test_expect_success 'incomplete sideband messages are reassembled' ' grep "Hello, world" err ' +test_expect_success 'eof on sideband message is reported' ' + printf 1234 >input && + test-tool pkt-line receive-sideband <input 2>err && + test_i18ngrep "unexpected disconnect" err +' + +test_expect_success 'missing sideband designator is reported' ' + printf 0004 >input && + test-tool pkt-line receive-sideband <input 2>err && + test_i18ngrep "missing sideband" err +' + test_done
diff --git a/pkt-line.c b/pkt-line.c index 657a702927..d633005ef7 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -461,9 +461,11 @@ int recv_sideband(const char *me, int in_stream, int out) enum sideband_type sideband_type; while (1) { - len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, - 0); - if (!demultiplex_sideband(me, buf, len, 0, &scratch, + int status = packet_read_with_status(in_stream, NULL, NULL, + buf, LARGE_PACKET_MAX, + &len, + PACKET_READ_GENTLE_ON_EOF); + if (!demultiplex_sideband(me, status, buf, len, 0, &scratch, &sideband_type)) continue; switch (sideband_type) { @@ -520,9 +522,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader) reader->options); if (!reader->use_sideband) break; - if (demultiplex_sideband(reader->me, reader->buffer, - reader->pktlen, 1, &scratch, - &sideband_type)) + if (demultiplex_sideband(reader->me, reader->status, + reader->buffer, reader->pktlen, 1, + &scratch, &sideband_type)) break; } diff --git a/sideband.c b/sideband.c index a5405b9aaa..a0d3fb9652 100644 --- a/sideband.c +++ b/sideband.c @@ -3,6 +3,7 @@ #include "config.h" #include "sideband.h" #include "help.h" +#include "pkt-line.h" struct keyword_entry { /* @@ -114,7 +115,8 @@ static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) #define ANSI_SUFFIX "\033[K" #define DUMB_SUFFIX " " -int demultiplex_sideband(const char *me, char *buf, int len, +int demultiplex_sideband(const char *me, int status, + char *buf, int len, int die_on_error, struct strbuf *scratch, enum sideband_type *sideband_type) @@ -130,17 +132,30 @@ int demultiplex_sideband(const char *me, char *buf, int len, suffix = DUMB_SUFFIX; } - if (len == 0) { - *sideband_type = SIDEBAND_FLUSH; - goto cleanup; - } - if (len < 1) { + if (status == PACKET_READ_EOF) { strbuf_addf(scratch, - "%s%s: protocol error: no band designator", + "%s%s: protocol error: eof while reading sideband packet", scratch->len ? "\n" : "", me); *sideband_type = SIDEBAND_PROTOCOL_ERROR; goto cleanup; } + + if (len < 0) + BUG("negative length on non-eof packet read"); + + if (len == 0) { + if (status == PACKET_READ_NORMAL) { + strbuf_addf(scratch, + "%s%s: protocol error: missing sideband designator", + scratch->len ? "\n" : "", me); + *sideband_type = SIDEBAND_PROTOCOL_ERROR; + } else { + /* covers flush, delim, etc */ + *sideband_type = SIDEBAND_FLUSH; + } + goto cleanup; + } + band = buf[0] & 0xff; buf[len] = '\0'; len--; diff --git a/sideband.h b/sideband.h index 227740a58e..5a25331be5 100644 --- a/sideband.h +++ b/sideband.h @@ -18,8 +18,12 @@ enum sideband_type { * * scratch must be a struct strbuf allocated by the caller. It is used to store * progress messages split across multiple packets. + * + * The "status" parameter is a pkt-line response as returned by + * packet_read_with_status() (e.g., PACKET_READ_NORMAL). */ -int demultiplex_sideband(const char *me, char *buf, int len, +int demultiplex_sideband(const char *me, int status, + char *buf, int len, int die_on_error, struct strbuf *scratch, enum sideband_type *sideband_type); diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 357201640a..936030a5ef 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -40,4 +40,16 @@ test_expect_success 'incomplete sideband messages are reassembled' ' grep "Hello, world" err ' +test_expect_success 'eof on sideband message is reported' ' + printf 1234 >input && + test-tool pkt-line receive-sideband <input 2>err && + test_i18ngrep "eof while reading" err +' + +test_expect_success 'missing sideband designator is reported' ' + printf 0004 >input && + test-tool pkt-line receive-sideband <input 2>err && + test_i18ngrep "missing sideband" err +' + test_done