Page MenuHomeFreeBSD

sbuf(9): Add sbuf_nl_terminate() API
ClosedPublic

Authored by cem on Jul 22 2019, 5:21 PM.

Details

Summary

The API is used to gracefully terminate lines with a single \n. If the
formatted buffer already ended in \n, it is left alone. If it did not, a
newline character is appended to it. The API, like other sbuf-modifying
routines, is only valid while the sbuf is not FINISHED.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Make sbuf_clear() case well-defined.

Logic looks good but I think the terminology is a little misleading. What the flag actually tracks is the last drained byte (mostly, see below). Including "drain" in the name somehow might make it more clear.

Do you plan to add a blurb to sbuf.9? Separately from this patch, it looks like the sbuf.9 blurb about functions that are incompatible with drains needs to be updated: _clear, _len, _bcpy, _cpy, ... any others?

sys/kern/subr_sbuf.c
229 ↗(On Diff #60026)

Worth a short comment, since there's actually no '\n' here?

391–408 ↗(On Diff #60026)

This seems correct, but it's a little non-obvious. It's relying on the idea that if we didn't drain the whole buf, then something remains in the buf, then we don't examine the flag, so it's okay to set it to garbage. I think it would be more clear to set it according to the value of the last byte that we actually drained (s->s_buf[len - 1] after a successful drain call).

746–749 ↗(On Diff #60026)

Do the predictions really help?

Logic looks good but I think the terminology is a little misleading. What the flag actually tracks is the last drained byte (mostly, see below). Including "drain" in the name somehow might make it more clear.

Well, it's a little more complicated than that; it's a yes/no bit for "if the buffer is currently empty, did we drain a non-newline character last?" Unfortunately, the usual pattern for documenting flags only leaves a handful of characters, so I did what I could in the space. If you'd prefer a longer flag comment, I can wrap the line and elaborate.

Do you plan to add a blurb to sbuf.9? Separately from this patch, it looks like the sbuf.9 blurb about functions that are incompatible with drains needs to be updated: _clear, _len, _bcpy, _cpy, ... any others?

I didn't, but that's a good idea, thanks. Will do.

sys/kern/subr_sbuf.c
229 ↗(On Diff #60026)

Yes, will add.

391–408 ↗(On Diff #60026)

The flag is really only backup when the buffer is fully drained and its contents are entirely invalid. If there are any contents in the buffer, the flag doesn't matter.

So, in this version it sometimes initializes it unnecessarily (because drain function *might* not drain the entire buffer); in your proposal, it still initializes it unnecessarily (if the drain function did not drain the entire buffer, we don't need to set the flag), just using the contents of a different byte. In both versions the flag value is invalidated by the remaining buffer contents anyway.

So my counter-proposal is to just move the existing logic inside the "fast path" if (s->s_len == 0) condition below. Will update.

746–749 ↗(On Diff #60026)

Probably not. They are inherited from the fmt version. I'm happy to drop 'em.

In D21030#456361, @cem wrote:

Logic looks good but I think the terminology is a little misleading. What the flag actually tracks is the last drained byte (mostly, see below). Including "drain" in the name somehow might make it more clear.

Well, it's a little more complicated than that; it's a yes/no bit for "if the buffer is currently empty, did we drain a non-newline character last?" Unfortunately, the usual pattern for documenting flags only leaves a handful of characters, so I did what I could in the space. If you'd prefer a longer flag comment, I can wrap the line and elaborate.

I might suggest something like SBUF_DRAINATEOL and corresponding _IS macro for consideration. But I don't feel strongly about it. Maybe just enhance the sbuf_nl_terminate comment some and add a comment pointer to it from sbuf_drain.

sys/kern/subr_sbuf.c
391–408 ↗(On Diff #60026)

Sounds good.

746–749 ↗(On Diff #60026)

Your call. IMHO: questionable benefit but definitely ugly.

cem marked 5 inline comments as done.
  • Address review feedback

Looks good to me.

share/man/man9/sbuf.9
127 ↗(On Diff #60059)

Just noting, local context uses .Fo / .Fa instead of .Fn here. I think they're equivalent.

This revision is now accepted and ready to land.Jul 23 2019, 8:59 PM
This revision was automatically updated to reflect the committed changes.