From owner-svn-src-head@FreeBSD.ORG Fri Mar 13 20:51:26 2015 Return-Path: Delivered-To: svn-src-head@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 0CC97DB6; Fri, 13 Mar 2015 20:51:26 +0000 (UTC) Received: from relay.mailchannels.net (aso-006-i443.relay.mailchannels.net [23.91.64.124]) by mx1.freebsd.org (Postfix) with ESMTP id ACC6E5EE; Fri, 13 Mar 2015 20:51:23 +0000 (UTC) X-Sender-Id: duocircle|x-authuser|hippie Received: from smtp3.ore.mailhop.org (ip-10-237-13-110.us-west-2.compute.internal [10.237.13.110]) by relay.mailchannels.net (Postfix) with ESMTPA id A172DA054F; Fri, 13 Mar 2015 20:42:24 +0000 (UTC) X-Sender-Id: duocircle|x-authuser|hippie Received: from smtp3.ore.mailhop.org (smtp3.ore.mailhop.org [10.21.145.197]) (using TLSv1 with cipher DHE-RSA-AES256-SHA) by 0.0.0.0:2500 (trex/5.4.8); Fri, 13 Mar 2015 20:42:25 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: duocircle|x-authuser|hippie X-MailChannels-Auth-Id: duocircle X-MC-Loop-Signature: 1426279344781:1644432435 X-MC-Ingress-Time: 1426279344781 Received: from c-73-34-117-227.hsd1.co.comcast.net ([73.34.117.227] helo=ilsoft.org) by smtp3.ore.mailhop.org with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.82) (envelope-from ) id 1YWWPW-0001yk-Vf; Fri, 13 Mar 2015 20:42:23 +0000 Received: from revolution.hippie.lan (revolution.hippie.lan [172.22.42.240]) by ilsoft.org (8.14.9/8.14.9) with ESMTP id t2DKgDBW017076; Fri, 13 Mar 2015 14:42:13 -0600 (MDT) (envelope-from ian@freebsd.org) X-Mail-Handler: DuoCircle Outbound SMTP X-Originating-IP: 73.34.117.227 X-Report-Abuse-To: abuse@duocircle.com (see https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information for abuse reporting information) X-MHO-User: U2FsdGVkX18e/+cDNCdM2PPKpfzfmt9O Message-ID: <1426279333.45674.11.camel@freebsd.org> Subject: Re: svn commit: r279932 - head/sys/vm From: Ian Lepore To: John Baldwin Date: Fri, 13 Mar 2015 14:42:13 -0600 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> Content-Type: multipart/mixed; boundary="=-NcRRSB3FPT93nPMFnfu0" X-Mailer: Evolution 3.12.10 FreeBSD GNOME Team Port Mime-Version: 1.0 X-AuthUser: hippie Cc: "svn-src-head@freebsd.org" , "svn-src-all@freebsd.org" , "src-committers@freebsd.org" , Ryan Stone X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Mar 2015 20:51:26 -0000 --=-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--