Skip site navigation (1)Skip section navigation (2)
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>