svn commit: r279932 - head/sys/vm

John Baldwin jhb at freebsd.org
Fri Mar 13 21:44:34 UTC 2015


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


More information about the svn-src-all mailing list