From owner-svn-src-all@FreeBSD.ORG Fri Mar 13 21:44:34 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 78EEADB; Fri, 13 Mar 2015 21:44:34 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 34D7EC0F; Fri, 13 Mar 2015 21:44:34 +0000 (UTC) Received: from ralph.baldwin.cx (pool-173-54-116-245.nwrknj.fios.verizon.net [173.54.116.245]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 3FEE0B945; Fri, 13 Mar 2015 17:44:31 -0400 (EDT) From: John Baldwin To: Ian Lepore Subject: Re: svn commit: r279932 - head/sys/vm Date: Fri, 13 Mar 2015 17:12:04 -0400 Message-ID: <2091019.TzmnaqNB4x@ralph.baldwin.cx> User-Agent: KMail/4.14.2 (FreeBSD/10.1-STABLE; KDE/4.14.2; amd64; ; ) 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> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 13 Mar 2015 17:44:31 -0400 (EDT) Cc: "svn-src-head@freebsd.org" , "svn-src-all@freebsd.org" , "src-committers@freebsd.org" , Ryan Stone X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Mar 2015 21:44:34 -0000 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