Date: Tue, 07 Sep 2010 19:40:39 +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 Message-ID: <49781.1283888439@critter.freebsd.dk> In-Reply-To: Your message of "Tue, 07 Sep 2010 11:12:55 MST." <AANLkTinReHpgm8Bt8ya=PY-3Mz6w0FwcGq-psrjiesKv@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <AANLkTinReHpgm8Bt8ya=PY-3Mz6w0FwcGq-psrjiesKv@mail.gmail.com>, mdf@ FreeBSD.org writes: Not so fast... The first patch (0001...) I am basically OK with. I question the choice of EOVERFLOW, which is generally for fixed size data structures. I think that should be changed to ENOMEM, pretty much throughout. I'm not sure I see much point in the second patch (0002...) but whatever. The third patch (0003...) Is not ok with me, for a number of reasons. First, I am not even sure we should add this to sbuf in the first place. Both userland and kernel printf's have half-assed support for random drain functions already. On the other hand, those functions are PITA to use and mostly undocumented hacks, so we should get something better constructed. Sbufs were designed to provide overflow-safe string operations on buffers (static or dynamic) with latching error detection. They were not designed to become a generalized I/O stream facility. By adding drain functionality to sbufs, we get significant overlap with stdio's FILE, but not anywhere close to being able to replace those. Is such near-duplication a good idea ? (Ohh, and while we're at it: If we modify sbufs anyway, should we add wchar_t support ?) If we decide to add drain function support to sbufs, I have issues with the suggested patches: For one thing, sbuf is not a kernel facility, it is a general purpose string editing library which is also used outside FreeBSD. If we add the drain facility it should work in both userland and kernel. The drain function does not, and should not have to know that sbufs are involved, it should have a prototype of: int sbuf_drain(void *private, const char *ptr, int len) Return value: >= 0 # bytes drained -1 error, errno relevante -2 error, errno not relevant Note: The the private argument should be a void*, not an uintptr_t This means that the autoextend can not be implemented in terms of a drain, but that looked plain wrong to me any way. The next thing is when do we call the drain function. If we add drain callbacks, we should do it right and offer all three canonical options: For every char When buffer is full. On line boundaries (ie: one or more lines) sbuf_flush() is unecessary, sbuf_finish() should be used, because that is the only way you can be sure the error code is available for checking afterwards. sbuf_unfinished_data() is just plain wrong and shall not be added, ever. The internals of sbufs are strictly private and should stay private. Sbuf does not make any promises about how the string is stored internally until you sbuf_finish() the buffer. By changing the drain prototype as per above, sbuf_unfinished_data() is not needed. Poul-Henning -- 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?49781.1283888439>