Re: [PATCH v3] Refactor recv_sideband()

[Nicolas Pitre <nico@fluxnic.net>]

On Wed, 22 Jun 2016, Nicolas Pitre wrote:

> On Wed, 22 Jun 2016, Lukas Fleischer wrote:
> 
> > Before this patch, we used character buffer manipulations to split
> > messages from the sideband at line breaks and insert "remote: " at the
> > beginning of each line, using the packet size to determine the end of a
> > message. However, since it is safe to assume that diagnostic messages
> > from the sideband never contain NUL characters, we can also
> > NUL-terminate the buffer, use strpbrk() for splitting lines and use
> > format strings to insert the prefix.
> > 
> > A static strbuf is used for constructing the output which is then
> > printed using a single write() call, such that the atomicity of the
> > output is preserved. See 9ac13ec (atomic write for sideband remote
> > messages, 2006-10-11) for details.
> > 
> > Helped-by: Nicolas Pitre <nico@fluxnic.net>
> > Signed-off-by: Lukas Fleischer <lfleischer@lfos.de>
> 
> The patch is buggy.
> 
> Once patched, the code looks like this:
> 
>         case 2:
>                 b = buf + 1;
>                 /*
>                  * Append a suffix to each nonempty line to clear the
>                  * end of the screen line.
>                  */
>                 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);
>                         }
>                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
>                         strbuf_reset(&outbuf);
>                         strbuf_addf(&outbuf, "%s", PREFIX);
>                         b = brk + 1;
>                 }
>                 if (*b) {
>                         xwrite(STDERR_FILENO, outbuf.buf, outbuf.len);
>                         /* Incomplete line, skip the next prefix. */
>                         strbuf_reset(&outbuf);
>                 }
>                 continue;
> 
> You are probably missing a strbuf_addf() before the last xwrite().

In fact, you could simply append the partial line to the strbuf and make 
it the prefix for the next packet rather than writing a partial line.  
You'd only have to write a partial line before leaving the function if 
the strbuf is not empty at that point.


Nicolas