Date: Fri, 13 Mar 2015 14:42:13 -0600 From: Ian Lepore <ian@freebsd.org> To: John Baldwin <jhb@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: <1426279333.45674.11.camel@freebsd.org> In-Reply-To: <1700119.KBm0D9PSuZ@ralph.baldwin.cx> References: <201503121806.t2CI6VSU034853@svn.freebsd.org> <3013452.2FfDYxpIKo@ralph.baldwin.cx> <1426269478.19693.4.camel@freebsd.org> <1700119.KBm0D9PSuZ@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-NcRRSB3FPT93nPMFnfu0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
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
--=-NcRRSB3FPT93nPMFnfu0
Content-Disposition: inline; filename="sbuf_includenul.diff"
Content-Type: text/x-patch; name="sbuf_includenul.diff"; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Index: sys/kern/kern_sysctl.c
===================================================================
--- sys/kern/kern_sysctl.c (revision 279962)
+++ sys/kern/kern_sysctl.c (working copy)
@@ -1807,7 +1807,7 @@ sbuf_new_for_sysctl(struct sbuf *s, char *buf, int
struct sysctl_req *req)
{
- s = sbuf_new(s, buf, length, SBUF_FIXEDLEN);
+ s = sbuf_new(s, buf, length, SBUF_FIXEDLEN | SBUF_INCLUDENUL);
sbuf_set_drain(s, sbuf_sysctl_drain, req);
return (s);
}
Index: sys/kern/subr_sbuf.c
===================================================================
--- sys/kern/subr_sbuf.c (revision 279962)
+++ sys/kern/subr_sbuf.c (working copy)
@@ -262,6 +262,28 @@ sbuf_uionew(struct sbuf *s, struct uio *uio, int *
}
#endif
+int
+sbuf_get_flags(struct sbuf *s)
+{
+
+ return (s->s_flags);
+}
+
+void
+sbuf_clear_flags(struct sbuf *s, int flags)
+{
+
+ s->s_flags &= ~(flags & SBUF_USRFLAGMSK);
+}
+
+void
+sbuf_set_flags(struct sbuf *s, int flags)
+{
+
+
+ s->s_flags |= (flags & SBUF_USRFLAGMSK);
+}
+
/*
* Clear an sbuf and reset its position.
*/
@@ -697,11 +719,13 @@ sbuf_finish(struct sbuf *s)
assert_sbuf_integrity(s);
assert_sbuf_state(s, 0);
+ s->s_buf[s->s_len] = '\0';
+ if (s->s_flags & SBUF_INCLUDENUL)
+ s->s_len++;
if (s->s_drain_func != NULL) {
while (s->s_len > 0 && s->s_error == 0)
s->s_error = sbuf_drain(s);
}
- s->s_buf[s->s_len] = '\0';
SBUF_SETFLAG(s, SBUF_FINISHED);
#ifdef _KERNEL
return (s->s_error);
@@ -743,6 +767,10 @@ sbuf_len(struct sbuf *s)
if (s->s_error != 0)
return (-1);
+
+ /* If finished, nulterm is already in len, else add one. */
+ if ((s->s_flags & (SBUF_INCLUDENUL | SBUF_FINISHED)) == SBUF_INCLUDENUL)
+ return (s->s_len + 1);
return (s->s_len);
}
Index: sys/sys/sbuf.h
===================================================================
--- sys/sys/sbuf.h (revision 279962)
+++ sys/sys/sbuf.h (working copy)
@@ -48,6 +48,7 @@ struct sbuf {
ssize_t s_len; /* current length of string */
#define SBUF_FIXEDLEN 0x00000000 /* fixed length buffer (default) */
#define SBUF_AUTOEXTEND 0x00000001 /* automatically extend buffer */
+#define SBUF_INCLUDENUL 0x00000002 /* nulterm byte is counted in len */
#define SBUF_USRFLAGMSK 0x0000ffff /* mask of flags the user may specify */
#define SBUF_DYNAMIC 0x00010000 /* s_buf must be freed */
#define SBUF_FINISHED 0x00020000 /* set by sbuf_finish() */
@@ -64,6 +65,9 @@ __BEGIN_DECLS
struct sbuf *sbuf_new(struct sbuf *, char *, int, int);
#define sbuf_new_auto() \
sbuf_new(NULL, NULL, 0, SBUF_AUTOEXTEND)
+int sbuf_get_flags(struct sbuf *);
+void sbuf_clear_flags(struct sbuf *, int);
+void sbuf_set_flags(struct sbuf *, int);
void sbuf_clear(struct sbuf *);
int sbuf_setpos(struct sbuf *, ssize_t);
int sbuf_bcat(struct sbuf *, const void *, size_t);
--=-NcRRSB3FPT93nPMFnfu0--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1426279333.45674.11.camel>