sbuf(9): Add sbuf_nl_terminate() API
ClosedPublic
Actions

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

Details

Reviewers

vangyzen
bapt
rlibby

Group Reviewers

manpages

Commits

rS350693: sbuf(9): Add sbuf_nl_terminate() API

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25472
Build 24091: arc lint + arc unit

Event Timeline

cem created this revision.Jul 22 2019, 5:21 PM

Harbormaster completed remote builds in B25458: Diff 60025.Jul 22 2019, 5:21 PM

Make sbuf_clear() case well-defined.

Harbormaster completed remote builds in B25459: Diff 60026.Jul 22 2019, 5:32 PM

cem added a reviewer: rlibby.Jul 22 2019, 5:34 PM

cem added a parent revision: D21029: sbuf(9): Refactor sbuf_newbuf into sbuf_new.

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	Worth a short comment, since there's actually no `'\n'` here?
394–411	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).
754–757	Do the predictions really help?

In D21030#456320, @rlibby 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.

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	Yes, will add.
394–411	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.
754–757	Probably not. They are inherited from the `fmt` version. I'm happy to drop 'em.

In D21030#456361, @cem wrote:

In D21030#456320, @rlibby 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.