Date: Fri, 13 Mar 2015 17:12:04 -0400 From: John Baldwin <jhb@freebsd.org> To: Ian Lepore <ian@freebsd.org> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Ryan Stone <rysto32@gmail.com> Subject: Re: svn commit: r279932 - head/sys/vm Message-ID: <2091019.TzmnaqNB4x@ralph.baldwin.cx> In-Reply-To: <1426279333.45674.11.camel@freebsd.org> References: <201503121806.t2CI6VSU034853@svn.freebsd.org> <1700119.KBm0D9PSuZ@ralph.baldwin.cx> <1426279333.45674.11.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, March 13, 2015 02:42:13 PM Ian Lepore wrote: > On Fri, 2015-03-13 at 14:34 -0400, John Baldwin wrote: > > On Friday, March 13, 2015 11:57:58 AM Ian Lepore wrote: > > > On Fri, 2015-03-13 at 13:19 -0400, John Baldwin wrote: > > > > On Friday, March 13, 2015 10:14:27 AM Ian Lepore wrote: > > > > > On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote: > > > > > > On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: > > > [...] > > > > > > > > > > In general I'm glad I got called away to an onsite meeting yesterday and > > > > > didn't get far with these changes, because the more I think about it, > > > > > the less satisfied I am with this expedient fix. The other fix I > > > > > started on, where a new SBUF_COUNTNUL flag can be set to inform the > > > > > sbuf_finish() code that you want the terminating nul counted in the data > > > > > length just feels like a better fit for the overall "automaticness" of > > > > > how the sbuf stuff works. > > > > > > > > Hmm, I actually think that it's a bug that the terminating nul isn't included > > > > when draining. If we fixed that then I think that fixes most of these? > > > > The places that explicitly use 'sysctl_handle_string()' with an sbuf > > > > should probably just be using sbuf_len(sb) + 1' explicitly. (Another > > > > option would be to have a sysctl_handle_sbuf() that was a wrapper around > > > > sysctl_handle_string() that included the + 1 to hide that detail if there is > > > > more than one.) > > > > > > > > > > Some of the uses of sbuf for sysctl use sbuf_bcat() for dealing with > > > binary structs, so we can't just assume that a nullterm should be added > > > and included in the buffer length -- there needs to be some mechanism to > > > say explicitly "this is an sbuf for a sysctl string" (and more generally > > > "this is an sbuf for a string where I want the nul byte counted as part > > > of the data" because that could be useful in non-sysctl contexts too, > > > especially in userland). > > > > Humm, that would seem to be an abuse of the API really. It is specifically > > designed for strings as someone else noted at the start of this thread (and > > as noted in the manpage). If anything I'd argue that the use cases that don't > > want a string should be the ones that should get a special flag in that case > > (or perhaps we should have a different little API to manage a buffer used for > > a draining sysctl where the data is a binary blob instead of a string). If > > you agree I'm happy to do some of the work (e.g. the different wrapper API). > > > > Given the existance of sbuf_bcpy() and sbuf_bcat() I'm not sure we can > say using sbuf for binary data is any kind of violation; somebody just > used the API that was provided to solve their problem. Well, it still nul-terminates the result of those, so I think it's still really dealing with strings. Those can be useful for appending non nul-terminated strings (for example using the d_namelen from a dirent). However, I think an INCLUDENUL flag is fine. It is a smaller change than adding a new sysctl-drain API. > Binary data is the exception in the sysctl case, and the idea of having > sbuf_new_for_sysctl() assume you're setting up to handle a sysctl string > and requiring the rare binary uses to do something different does make a > lot of sense. That might lead to a patch like the one below, which > would automatically fix most of the current sysctl sbuf users, and the 2 > or 3 places that are using binary data would need to add a line: > > sbuf_clear_flags(&sbuf, SBUF_INCLUDENUL); > > I should mention too that the larger problem I'm trying to clean up is > that some sysctl strings include the nul byte in the data returned to > userland and some don't. There are more direct callers of SYSCTL_OUT() > that fail to add a nulterm, I have a whole separate set of fixes for > those, but I'm becoming somewhat inclined to fix them by converting them > to use sbuf and just make that the established idiom for returning > dynamic strings via sysctl. Either that or using sysctl_handle_string() when possible instead of bare SYSCTL_OUT(). I think your patch is fine. I agree this will be a much smaller change to roll out. :) -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2091019.TzmnaqNB4x>