Re: [PATCH v4] Refactor recv_sideband()

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