Extending sbufs with a drain, take 2

[Poul-Henning Kamp]

In message <AANLkTim16YitneEE7kkWzwssQasab3qG9dygv+TuHa7x at mail.gmail.com>, mdf@
FreeBSD.org writes:

>After the discussion here with phk, I have reworked my patches. 

Much better, but still not there IMO.

>Refactor sbuf code so that most uses of sbuf_extend() are in a new
>sbuf_put_byte().  This makes it easier to add drain functionality when a
>buffer would overflow as there are fewer code points.
>
>http://people.freebsd.org/~mdf/0001-Refactor-sbuf-code-so-that-most-uses-of-sbuf_extend-.patch

I would probably have made sbuf_put_byte() a proper sbuf function and
added a one-line wrapper function for the benefit of kvprintf(), rather
than loose type-safety for all the other functions.

>Fix small errors in the sbuf(9) man page.
>
>http://people.freebsd.org/~mdf/0002-Fix-small-errors-in-the-sbuf-9-man-page.patch

Good catch.

>Add drain functionality to sbufs.  The drain is a function that is
>called when the sbuf internal buffer is filled.  For kernel sbufs with a
>drain, the internal buffer will never be expanded.  For userland sbufs
>with a drain, the internal buffer may still be expanded by
>sbuf_[v]printf(3).
>
>http://people.freebsd.org/~mdf/0003-Add-drain-functionality-to-sbufs.-The-drain-is-a-fun.patch

I'm not sure I can see the point of the flags argument.  I would just have
sbuf_finish() keep calling the drain until everything is gone, that is
what is going to happen for any sbuf_foo() call if the drain function
only picks one byte at a time.

If you need any "magic EOF" processing, you can do that from the
main code-path after calling sbuf_finish(), since you have already
stipulated:

]] +The returned drained length cannot be zero.

In regards to manpage:

]] @@ -311,6 +351,9 @@ functions return the actual string and its length, respectively;
]]  .Fn sbuf_data
]]  only works on a finished
]]  .Fa sbuf .
]] +Neither function should be used on an
]] +.Fa sbuf
]] +with an attached drain.
]]  .Fn sbuf_done
]]  returns non-zero if the
]]  .Fa sbuf

I thought we agreed that sbuf_len() returned #undrained_bytes ?

Instead of spreading it out over the text you should add a new
section where you describe the workings of drains, including details
such as the "len=2" for char-by-char drain, return values and
requirements of a drain fucntion, and list the functions that cannot
be used on these sbufs.  (like sbuf_trim() ?)

sbuf_set_drain():
-----------------

You assert that the drain function can not be overwritten, that
seems excessive to me ?  Why wouldn't that be allowed ?

I would only assert that to change in and out of a drain
mode the sbuf must be empty:

	if ((s->drain_func == NULL && func != NULL) ||
	    (s->drain_func != NULL && func == NULL))
		assert(s->s_len == 0);


>Add a drain function for struct sysctl_req, and use it for a variety
>of handlers that had to do awkward things to get a large enough
>FIXEDLEN buffer.
>
>http://people.freebsd.org/~mdf/0004-Add-a-drain-function-for-struct-sysctl_req-and-use-i.patch

I sort of think sbuf_sysctl_drain(), sbuf_new_for_sysctl() belongs in
kern_sysctl.c more than in subr_sbuf.c, mostly in view of the shared
kernel/userland aspect of sbufs.

I Love patches which do remove twice as much code as they add:
	 12 files changed, 105 insertions(+), 196 deletions(-)

>Fix an incorrect use of sbuf_overflowed() after a call to sbuf_finish().
>
>http://people.freebsd.org/~mdf/0005-Fix-an-incorrect-use-of-sbuf_overflowed-after-a-call.patch
>
>---
>
>Replace sbuf_overflowed() with sbuf_error(), which returns any error
>code associated with overflow or with the drain function.
>
>http://people.freebsd.org/~mdf/0006-Replace-sbuf_overflowed-with-sbuf_error-which-return.patch

Now that sbuf_finish() returns the error, most applications should
not need to sbuf_finish()/sbuf_error() at all.

I can see a valid use for sbuf_error() in this sort of code:

	sb = sbuf_new(...)
	for (really long and tortous loop) {
		sbuf_bla(sb, ....)
		...
		if (sbuf_error(sb))
			break;	 /* Don't waste time if we lost already */
	}

But the normal case now should just be to check the return of
sbuf_finish().

-- 
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
phk at FreeBSD.ORG         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.