From owner-freebsd-arch@FreeBSD.ORG Thu Sep 9 07:27:01 2010 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1C4DE10656CC; Thu, 9 Sep 2010 07:27:01 +0000 (UTC) (envelope-from phk@critter.freebsd.dk) Received: from phk.freebsd.dk (phk.freebsd.dk [130.225.244.222]) by mx1.freebsd.org (Postfix) with ESMTP id A13DB8FC14; Thu, 9 Sep 2010 07:27:00 +0000 (UTC) Received: from critter.freebsd.dk (critter.freebsd.dk [192.168.61.3]) by phk.freebsd.dk (Postfix) with ESMTP id 418843F61A; Thu, 9 Sep 2010 07:26:59 +0000 (UTC) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.14.4/8.14.4) with ESMTP id o897QxSu085036; Thu, 9 Sep 2010 07:26:59 GMT (envelope-from phk@critter.freebsd.dk) To: mdf@FreeBSD.org From: "Poul-Henning Kamp" In-Reply-To: Your message of "Wed, 08 Sep 2010 16:14:59 MST." Date: Thu, 09 Sep 2010 07:26:59 +0000 Message-ID: <85035.1284017219@critter.freebsd.dk> Sender: phk@critter.freebsd.dk Cc: FreeBSD Arch Subject: Re: Extending sbufs with a drain, take 2 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Sep 2010 07:27:01 -0000 In message , 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.