Date: Tue, 7 May 2019 10:52:20 -0700 (PDT) From: "Rodney W. Grimes" <freebsd@gndrsh.dnsmgr.net> To: Conrad Meyer <cem@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r347229 - in head: lib/libsbuf lib/libsbuf/tests share/man/man9 sys/kern sys/sys Message-ID: <201905071752.x47HqK9n012716@gndrsh.dnsmgr.net> In-Reply-To: <201905071747.x47HlKTh012163@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> Author: cem > Date: Tue May 7 17:47:20 2019 > New Revision: 347229 > URL: https://svnweb.freebsd.org/changeset/base/347229 > > Log: > device_printf: Use sbuf for more coherent prints on SMP > > device_printf does multiple calls to printf allowing other console messages to > be inserted between the device name, and the rest of the message. This change > uses sbuf to compose to two into a single buffer, and prints it all at once. > > It exposes an sbuf drain function (drain-to-printf) for common use. > > Update documentation to match; some unit tests included. > > Submitted by: jmg > Sponsored by: Dell EMC Isilon > Differential Revision: https://reviews.freebsd.org/D16690 Thank you! this has been annoying me for a while, I knew it was going on, but wasnt sure where it was coming from. Does this code MFC back to 12 and 11 easily? > Modified: > head/lib/libsbuf/Symbol.map > head/lib/libsbuf/tests/sbuf_core_test.c > head/lib/libsbuf/tests/sbuf_stdio_test.c > head/share/man/man9/Makefile > head/share/man/man9/sbuf.9 > head/sys/kern/kern_sysctl.c > head/sys/kern/subr_bus.c > head/sys/kern/subr_prf.c > head/sys/sys/sbuf.h > > Modified: head/lib/libsbuf/Symbol.map > ============================================================================== > --- head/lib/libsbuf/Symbol.map Tue May 7 16:17:33 2019 (r347228) > +++ head/lib/libsbuf/Symbol.map Tue May 7 17:47:20 2019 (r347229) > @@ -37,5 +37,6 @@ FBSD_1.4 { > > FBSD_1.5 { > sbuf_putbuf; > + sbuf_printf_drain; > }; > > > Modified: head/lib/libsbuf/tests/sbuf_core_test.c > ============================================================================== > --- head/lib/libsbuf/tests/sbuf_core_test.c Tue May 7 16:17:33 2019 (r347228) > +++ head/lib/libsbuf/tests/sbuf_core_test.c Tue May 7 17:47:20 2019 (r347229) > @@ -63,6 +63,9 @@ ATF_TC_BODY(sbuf_clear_test, tc) > */ > child_proc = atf_utils_fork(); > if (child_proc == 0) { > + ATF_REQUIRE_EQ_MSG(0, sbuf_finish(sb), "sbuf_finish failed: %s", > + strerror(errno)); > + > sbuf_putbuf(sb); > exit(0); > } > @@ -100,6 +103,34 @@ ATF_TC_BODY(sbuf_done_and_sbuf_finish_test, tc) > sbuf_delete(sb); > } > > +static int > +drain_ret0(void *arg, const char *data, int len) > +{ > + > + (void)arg; > + (void)data; > + (void)len; > + > + return (0); > +} > + > +ATF_TC_WITHOUT_HEAD(sbuf_drain_ret0_test); > +ATF_TC_BODY(sbuf_drain_ret0_test, tc) > +{ > + struct sbuf *sb; > + > + sb = sbuf_new_auto(); > + > + sbuf_set_drain(sb, drain_ret0, NULL); > + > + sbuf_cat(sb, test_string); > + > + ATF_CHECK_EQ_MSG(-1, sbuf_finish(sb), > + "required to return error when drain func returns 0"); > + ATF_CHECK_EQ_MSG(EDEADLK, errno, > + "errno required to be EDEADLK when drain func returns 0"); > +} > + > ATF_TC_WITHOUT_HEAD(sbuf_len_test); > ATF_TC_BODY(sbuf_len_test, tc) > { > @@ -131,6 +162,34 @@ ATF_TC_BODY(sbuf_len_test, tc) > sbuf_delete(sb); > } > > +ATF_TC_WITHOUT_HEAD(sbuf_new_fixedlen); > +ATF_TC_BODY(sbuf_new_fixedlen, tc) > +{ > + char buf[strlen(test_string) + 1]; > + struct sbuf sb; > + pid_t child_proc; > + > + sbuf_new(&sb, buf, sizeof(buf), SBUF_FIXEDLEN); > + > + sbuf_cat(&sb, test_string); > + > + child_proc = atf_utils_fork(); > + if (child_proc == 0) { > + ATF_REQUIRE_EQ_MSG(0, sbuf_finish(&sb), "sbuf_finish failed: %s", > + strerror(errno)); > + > + sbuf_putbuf(&sb); > + exit(0); > + } > + atf_utils_wait(child_proc, 0, test_string, ""); > + > + sbuf_putc(&sb, ' '); > + > + ATF_CHECK_EQ_MSG(-1, sbuf_finish(&sb), "failed to return error on overflow"); > + > + sbuf_delete(&sb); > +} > + > ATF_TC_WITHOUT_HEAD(sbuf_setpos_test); > ATF_TC_BODY(sbuf_setpos_test, tc) > { > @@ -190,7 +249,9 @@ ATF_TP_ADD_TCS(tp) > > ATF_TP_ADD_TC(tp, sbuf_clear_test); > ATF_TP_ADD_TC(tp, sbuf_done_and_sbuf_finish_test); > + ATF_TP_ADD_TC(tp, sbuf_drain_ret0_test); > ATF_TP_ADD_TC(tp, sbuf_len_test); > + ATF_TP_ADD_TC(tp, sbuf_new_fixedlen); > #if 0 > /* TODO */ > #ifdef HAVE_SBUF_CLEAR_FLAGS > > Modified: head/lib/libsbuf/tests/sbuf_stdio_test.c > ============================================================================== > --- head/lib/libsbuf/tests/sbuf_stdio_test.c Tue May 7 16:17:33 2019 (r347228) > +++ head/lib/libsbuf/tests/sbuf_stdio_test.c Tue May 7 17:47:20 2019 (r347229) > @@ -59,6 +59,60 @@ sbuf_vprintf_helper(struct sbuf *sb, const char * rest > return (rc); > } > > +ATF_TC_WITHOUT_HEAD(sbuf_printf_drain_null_test); > +ATF_TC_BODY(sbuf_printf_drain_null_test, tc) > +{ > + struct sbuf *sb; > + char buf[2]; > + pid_t child_proc; > + > + sb = sbuf_new(NULL, buf, sizeof(buf), SBUF_FIXEDLEN); > + ATF_REQUIRE_MSG(sb != NULL, "sbuf_new_auto failed: %s", > + strerror(errno)); > + > + child_proc = atf_utils_fork(); > + if (child_proc == 0) { > + sbuf_set_drain(sb, sbuf_printf_drain, NULL); > + > + ATF_REQUIRE_EQ_MSG(0, sbuf_cat(sb, test_string), > + "sbuf_cat failed"); > + > + ATF_CHECK_EQ(0, sbuf_finish(sb)); > + exit(0); > + } > + atf_utils_wait(child_proc, 0, test_string, ""); > + > + sbuf_delete(sb); > +} > + > +ATF_TC_WITHOUT_HEAD(sbuf_printf_drain_test); > +ATF_TC_BODY(sbuf_printf_drain_test, tc) > +{ > + struct sbuf *sb; > + char buf[2]; > + pid_t child_proc; > + size_t cnt = 0; > + > + sb = sbuf_new(NULL, buf, sizeof(buf), SBUF_FIXEDLEN); > + ATF_REQUIRE_MSG(sb != NULL, "sbuf_new_auto failed: %s", > + strerror(errno)); > + > + child_proc = atf_utils_fork(); > + if (child_proc == 0) { > + sbuf_set_drain(sb, sbuf_printf_drain, &cnt); > + > + ATF_REQUIRE_EQ_MSG(0, sbuf_cat(sb, test_string), > + "sbuf_cat failed"); > + > + ATF_CHECK_EQ(0, sbuf_finish(sb)); > + ATF_CHECK_EQ(strlen(test_string), cnt); > + exit(0); > + } > + atf_utils_wait(child_proc, 0, test_string, ""); > + > + sbuf_delete(sb); > +} > + > ATF_TC_WITHOUT_HEAD(sbuf_printf_test); > ATF_TC_BODY(sbuf_printf_test, tc) > { > @@ -106,6 +160,7 @@ ATF_TC_BODY(sbuf_putbuf_test, tc) > > child_proc = atf_utils_fork(); > if (child_proc == 0) { > + ATF_CHECK_EQ(0, sbuf_finish(sb)); > sbuf_putbuf(sb); > exit(0); > } > @@ -152,6 +207,8 @@ ATF_TC_BODY(sbuf_vprintf_test, tc) > ATF_TP_ADD_TCS(tp) > { > > + ATF_TP_ADD_TC(tp, sbuf_printf_drain_null_test); > + ATF_TP_ADD_TC(tp, sbuf_printf_drain_test); > ATF_TP_ADD_TC(tp, sbuf_printf_test); > ATF_TP_ADD_TC(tp, sbuf_putbuf_test); > ATF_TP_ADD_TC(tp, sbuf_vprintf_test); > > Modified: head/share/man/man9/Makefile > ============================================================================== > --- head/share/man/man9/Makefile Tue May 7 16:17:33 2019 (r347228) > +++ head/share/man/man9/Makefile Tue May 7 17:47:20 2019 (r347229) > @@ -1780,6 +1780,8 @@ MLINKS+=sbuf.9 sbuf_bcat.9 \ > sbuf.9 sbuf_new_auto.9 \ > sbuf.9 sbuf_new_for_sysctl.9 \ > sbuf.9 sbuf_printf.9 \ > + sbuf.9 sbuf_printf_drain.9 \ > + sbuf.9 sbuf_putbuf.9 \ > sbuf.9 sbuf_putc.9 \ > sbuf.9 sbuf_set_drain.9 \ > sbuf.9 sbuf_set_flags.9 \ > > Modified: head/share/man/man9/sbuf.9 > ============================================================================== > --- head/share/man/man9/sbuf.9 Tue May 7 16:17:33 2019 (r347228) > +++ head/share/man/man9/sbuf.9 Tue May 7 17:47:20 2019 (r347229) > @@ -25,7 +25,7 @@ > .\" > .\" $FreeBSD$ > .\" > -.Dd May 23, 2018 > +.Dd May 7, 2019 > .Dt SBUF 9 > .Os > .Sh NAME > @@ -58,6 +58,7 @@ > .Nm sbuf_start_section , > .Nm sbuf_end_section , > .Nm sbuf_hexdump , > +.Nm sbuf_printf_drain , > .Nm sbuf_putbuf > .Nd safe string composition > .Sh SYNOPSIS > @@ -191,6 +192,12 @@ > .Fa "const char *hdr" > .Fa "int flags" > .Fc > +.Ft int > +.Fo sbuf_printf_drain > +.Fa "void *arg" > +.Fa "const char *data" > +.Fa "int len" > +.Fc > .Ft void > .Fo sbuf_putbuf > .Fa "struct sbuf *s" > @@ -278,7 +285,9 @@ as a record boundary marker that will be used during d > records being split. > If a record grows sufficiently large such that it fills the > .Fa sbuf > -and therefore cannot be drained without being split, an error of EDEADLK is set. > +and therefore cannot be drained without being split, an error of > +.Er EDEADLK > +is set. > .El > .Pp > Note that if > @@ -419,7 +428,9 @@ many bytes were drained. > If negative, the return value indicates the negative error code which > will be returned from this or a later call to > .Fn sbuf_finish . > -The returned drained length cannot be zero. > +If the returned drained length is 0, an error of > +.Er EDEADLK > +is set. > To do unbuffered draining, initialize the sbuf with a two-byte buffer. > The drain will be called for every byte added to the sbuf. > The > @@ -492,7 +503,9 @@ The > .Fn sbuf_error > function returns any error value that the > .Fa sbuf > -may have accumulated, either from the drain function, or ENOMEM if the > +may have accumulated, either from the drain function, or > +.Er ENOMEM > +if the > .Fa sbuf > overflowed. > This function is generally not needed and instead the error code from > @@ -574,9 +587,29 @@ See the > man page for more details on the interface. > .Pp > The > +.Fn sbuf_printf_drain > +function is a drain function that will call printf, or log to the console. > +The argument > +.Fa arg > +must be either > +.Dv NULL , > +or a valid pointer to a > +.Vt size_t . > +If > +.Fa arg > +is not > +.Dv NULL , > +the total bytes drained will be added to the value pointed to by > +.Fa arg . > +.Pp > +The > .Fn sbuf_putbuf > function printfs the sbuf to stdout if in userland, and to the console > and log if in the kernel. > +The > +.Fa sbuf > +must be finished before calling > +.Fn sbuf_putbuf . > It does not drain the buffer or update any pointers. > .Sh NOTES > If an operation caused an > @@ -655,8 +688,9 @@ function returns the section length or \-1 if the buff > .Pp > The > .Fn sbuf_finish 9 > -function (the kernel version) returns ENOMEM if the sbuf overflowed before > -being finished, > +function (the kernel version) returns > +.Er ENOMEM > +if the sbuf overflowed before being finished, > or returns the error code from the drain if one is attached. > .Pp > The > > Modified: head/sys/kern/kern_sysctl.c > ============================================================================== > --- head/sys/kern/kern_sysctl.c Tue May 7 16:17:33 2019 (r347228) > +++ head/sys/kern/kern_sysctl.c Tue May 7 17:47:20 2019 (r347229) > @@ -322,13 +322,6 @@ sysctl_load_tunable_by_oid_locked(struct sysctl_oid *o > freeenv(penv); > } > > -static int > -sbuf_printf_drain(void *arg __unused, const char *data, int len) > -{ > - > - return (printf("%.*s", len, data)); > -} > - > /* > * Locate the path to a given oid. Returns the length of the resulting path, > * or -1 if the oid was not found. nodes must have room for CTL_MAXNAME > > Modified: head/sys/kern/subr_bus.c > ============================================================================== > --- head/sys/kern/subr_bus.c Tue May 7 16:17:33 2019 (r347228) > +++ head/sys/kern/subr_bus.c Tue May 7 17:47:20 2019 (r347229) > @@ -2431,13 +2431,31 @@ device_print_prettyname(device_t dev) > int > device_printf(device_t dev, const char * fmt, ...) > { > + char buf[128]; > + struct sbuf sb; > + const char *name; > va_list ap; > - int retval; > + size_t retval; > > - retval = device_print_prettyname(dev); > + retval = 0; > + > + sbuf_new(&sb, buf, sizeof(buf), SBUF_FIXEDLEN); > + sbuf_set_drain(&sb, sbuf_printf_drain, &retval); > + > + name = device_get_name(dev); > + > + if (name == NULL) > + sbuf_cat(&sb, "unknown: "); > + else > + sbuf_printf(&sb, "%s%d: ", name, device_get_unit(dev)); > + > va_start(ap, fmt); > - retval += vprintf(fmt, ap); > + sbuf_vprintf(&sb, fmt, ap); > va_end(ap); > + > + sbuf_finish(&sb); > + sbuf_delete(&sb); > + > return (retval); > } > > > Modified: head/sys/kern/subr_prf.c > ============================================================================== > --- head/sys/kern/subr_prf.c Tue May 7 16:17:33 2019 (r347228) > +++ head/sys/kern/subr_prf.c Tue May 7 17:47:20 2019 (r347229) > @@ -62,6 +62,8 @@ __FBSDID("$FreeBSD$"); > #include <sys/syslog.h> > #include <sys/cons.h> > #include <sys/uio.h> > +#else /* !_KERNEL */ > +#include <errno.h> > #endif > #include <sys/ctype.h> > #include <sys/sbuf.h> > @@ -1254,3 +1256,42 @@ sbuf_putbuf(struct sbuf *sb) > printf("%s", sbuf_data(sb)); > } > #endif > + > +int > +sbuf_printf_drain(void *arg, const char *data, int len) > +{ > + size_t *retvalptr; > + int r; > +#ifdef _KERNEL > + char *dataptr; > + char oldchr; > + > + /* > + * This is allowed as an extra byte is always resvered for > + * terminating NUL byte. Save and restore the byte because > + * we might be flushing a record, and there may be valid > + * data after the buffer. > + */ > + oldchr = data[len]; > + dataptr = __DECONST(char *, data); > + dataptr[len] = '\0'; > + > + prf_putbuf(dataptr, TOLOG | TOCONS, -1); > + r = len; > + > + dataptr[len] = oldchr; > + > +#else /* !_KERNEL */ > + > + r = printf("%.*s", len, data); > + if (r < 0) > + return (-errno); > + > +#endif > + > + retvalptr = arg; > + if (retvalptr != NULL) > + *retvalptr += r; > + > + return (r); > +} > > Modified: head/sys/sys/sbuf.h > ============================================================================== > --- head/sys/sys/sbuf.h Tue May 7 16:17:33 2019 (r347228) > +++ head/sys/sys/sbuf.h Tue May 7 17:47:20 2019 (r347229) > @@ -103,6 +103,7 @@ void sbuf_start_section(struct sbuf *, ssize_t *); > ssize_t sbuf_end_section(struct sbuf *, ssize_t, size_t, int); > void sbuf_hexdump(struct sbuf *, const void *, int, const char *, > int); > +int sbuf_printf_drain(void *arg, const char *data, int len); > void sbuf_putbuf(struct sbuf *); > > #ifdef _KERNEL > > -- Rod Grimes rgrimes@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201905071752.x47HqK9n012716>