Date: Thu, 09 Sep 2010 07:26:59 +0000 From: "Poul-Henning Kamp" <phk@phk.freebsd.dk> To: mdf@FreeBSD.org Cc: FreeBSD Arch <freebsd-arch@FreeBSD.org> Subject: Re: Extending sbufs with a drain, take 2 Message-ID: <85035.1284017219@critter.freebsd.dk> In-Reply-To: Your message of "Wed, 08 Sep 2010 16:14:59 MST." <AANLkTim16YitneEE7kkWzwssQasab3qG9dygv%2BTuHa7x@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <AANLkTim16YitneEE7kkWzwssQasab3qG9dygv+TuHa7x@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@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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?85035.1284017219>