svn commit: r279932 - head/sys/vm

[Ian Lepore]

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.

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.

-- Ian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbuf_includenul.diff
Type: text/x-patch
Size: 2819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20150313/9f72c9a6/attachment.bin>