Re: [PATCH v4] Refactor recv_sideband()

Nicolas Pitre <nico@fluxnic.net> writes:

Unfortunately, that is not it.

The basic structure of the code (without the "SQUASH" we discussed) looks like this:

	strbuf_addf(&outbuf, "%s", PREFIX);
	while (retval == 0) {
		len = packet_read(in_stream, NULL, NULL, buf, LARGE_PACKET_MAX, 0);
		...
		band = buf[0] & 0xff;
		switch (band) {
		case 3:
			... /* emergency exit */
		case 2:
			while ((brk = strpbrk(b, "\n\r"))) {
				int linelen = brk - b;

				if (linelen > 0) {
					strbuf_addf(&outbuf, "%.*s%s%c",
						    linelen, b, suffix, *brk);
				} else {
					strbuf_addf(&outbuf, "%c", *brk);
				}
				fprintf(stderr, "%.*s", (int)outbuf.len,
					outbuf.buf);
				strbuf_reset(&outbuf);
				strbuf_addf(&outbuf, "%s", PREFIX);
				b = brk + 1;
			}
			if (*b)
				strbuf_addf(&outbuf, "%s", b);
			break;
		...
		}
	}

	if (outbuf.len > 0)
		fprintf(stderr, "%.*s", (int)outbuf.len, outbuf.buf);

Imagine we are reading from band #2 and we find a complete line. We concatenate the payload up to the LF at the end of the line to the PREFIX we prepared outside the loop and emit it, and then we ASSUME that we further have something after strpbrk() and add PREFIX to the buffer, before going to the next line in the payload.

But there may not be anything after the LF. outbuf.len is still counting the PREFIX and we end up showing it, without termination.

This takes us back to what I said in my review of an earlier round, in $gmane/297332, where I said:

    Instead of doing "we assume outbuf already has PREFIX when we add
    contents from buf[]", the code structure would be better if you:

     * make outbuf.buf contain PREFIX at the beginning of this innermost
       loop; lose the reset/addf from here.

     * move strbuf_reset(&outbuf) at the end of the next if (*b) block
       to just before "continue;"

    perhaps?

I think the strbuf_addf(PREFIX) above the loop should be removed, and instead the code should use the PREFIX only when it decides that there is something worth emitting, i.e.

	while (!retval) {
        	len = packet_read();
                ...
                band = buf[0] & 0xff;
                switch (band) {
                case 3:
                	... /* emergency exit */
		case 2:
                	while ((brk = ...)) {
                        	/* we have something to say */
				strbuf_reset(&outbuf);
                                strbuf_addstr(&outbuf, PREFIX);
                                if (linelen)
                                	strbuf_addf(...);
				else
                                	strbuf_addch(*brk);
				fwrite(outbuf.buf, 1, outbuf.len, stderr);
				b = brk + 1;
			}
                        if (*b) {
                        	/* we still have something to say */
				strbuf_reset(&outbuf);
                                strbuf_addstr(&outbuf, PREFIX);
                               	strbuf_addf(...);
			}
                        break;
		...
                }
	}