On Tue, 28 Jun 2016, Junio C Hamano wrote:
> And then that made me stare at the patch even more. We still write
> some error messages to stderr in the updated code (without my crap
> SQUASH) inside "while (!retval)" loop:
>
> while (retval == 0) {
> int band, len;
> len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
> if (len == 0)
> break;
> if (len < 1) {
> fprintf(stderr, "%s: protocol error: no band designator\n", me);
> retval = SIDEBAND_PROTOCOL_ERROR;
> break;
> }
> band = buf[0] & 0xff;
> buf[len] = '\0';
> len--;
> switch (band) {
> case 3:
> fprintf(stderr, "%s%s\n", PREFIX, buf + 1);
> retval = SIDEBAND_REMOTE_ERROR;
> break;
> case 2:
> ...
> while ((brk = strpbrk(b, "\n\r"))) {
> ...
> write(2, outbuf.buf, outbuf.len);
> ...
> }
>
> if (*b)
> strbuf_addf(&outbuf, "%s", b);
> break;
> case 1:
> write_or_die(out, buf + 1, len);
> break;
> default:
> fprintf(stderr, "%s: protocol error: bad band #%d\n",
> me, band);
> retval = SIDEBAND_PROTOCOL_ERROR;
> break;
> }
> }
>
> if (outbuf.len > 0)
> write(2, outbuf.buf, outbuf.len);
>
> In general, mixing stdio and raw file descriptor access is not such
> a good idea, but these remaining calls to fprintf(stderr, ...) above
> are for error-exit codepath, so from that point of view, the above
> illustration may be acceptable, but there is still one niggle.
>
> When we exit the loop because we set retval to a non-zero value,
> should we still drain the outbuf?
I would think so. Anything that the remote sent before any error should
be printed nevertheless. The clue for the error might be in the pending
buffer.
However in this case the actual error printout and the pending buffer
will appear reversed.
So what I'd suggest is actually something like this:
if (len < 1) {
strbuf_addf(&outbuf, "\n%s: protocol error: no band designator\n", me);
retval = SIDEBAND_PROTOCOL_ERROR;
break;
And so on for the other error cases.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html