On Mon, Oct 16, 2023 at 12:19:17PM +0200, Claudio Jeker wrote:
> I dislike how log.c does all these asprintf() calls with dubious
> workaround calls in case asprintf() fails.
You're not alone.
> IMO it is easier to use the stdio provided buffers and fflush() to get
> "atomic" writes on stderr. At least from my understanding this is the
> reason to go all this lenght to do a single fprintf call.
This makes sense, but I don't know the history here.
> We need this since in privsep daemons can log from multiple processes
> concurrently and we want the log to be not too mangled.
>
> While there also use one static buffer to handle log_warn() and vfatalc()
> where the error message is first expanded and then printed with
> logit(LOG_ERR, "%s: %s", fmtbuf, strerror(saved_errno));
>
> Any opinions?
I like this much better than the existing code.
ok tb, one small request inline
> --
> :wq Claudio
>
> Index: log.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/log.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 log.c
> --- log.c 21 Mar 2017 12:06:55 -0000 1.64
> +++ log.c 16 Oct 2023 09:42:55 -0000
> @@ -29,6 +29,7 @@
> static int debug;
> static int verbose;
> static const char *log_procname;
> +static char logbuf[4096], fmtbuf[4096];
>
> void
> log_init(int n_debug, int facility)
> @@ -41,6 +42,8 @@ log_init(int n_debug, int facility)
>
> if (!debug)
> openlog(__progname, LOG_PID | LOG_NDELAY, facility);
> + else
> + setvbuf(stderr, logbuf, _IOFBF, sizeof(logbuf));
>
> tzset();
> }
> @@ -77,18 +80,11 @@ logit(int pri, const char *fmt, ...)
> void
> vlog(int pri, const char *fmt, va_list ap)
> {
> - char *nfmt;
> int saved_errno = errno;
>
> if (debug) {
Could log_init() and vlog() either both use if (debug) or both if (!debug),
please? Missing the ! in log_init() confused me for a few minutes since it
looked wrong.
> - /* best effort in out of mem situations */
> - if (asprintf(&nfmt, "%s\n", fmt) == -1) {
> - vfprintf(stderr, fmt, ap);
> - fprintf(stderr, "\n");
> - } else {
> - vfprintf(stderr, nfmt, ap);
> - free(nfmt);
> - }
> + vfprintf(stderr, fmt, ap);
> + fprintf(stderr, "\n");
> fflush(stderr);
> } else
> vsyslog(pri, fmt, ap);
> @@ -99,7 +95,6 @@ vlog(int pri, const char *fmt, va_list a
> void
> log_warn(const char *emsg, ...)
> {
> - char *nfmt;
> va_list ap;
> int saved_errno = errno;
>
> @@ -108,16 +103,8 @@ log_warn(const char *emsg, ...)
> logit(LOG_ERR, "%s", strerror(saved_errno));
> else {
> va_start(ap, emsg);
> -
> - if (asprintf(&nfmt, "%s: %s", emsg,
> - strerror(saved_errno)) == -1) {
> - /* we tried it... */
> - vlog(LOG_ERR, emsg, ap);
> - logit(LOG_ERR, "%s", strerror(saved_errno));
> - } else {
> - vlog(LOG_ERR, nfmt, ap);
> - free(nfmt);
> - }
> + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> + logit(LOG_ERR, "%s: %s", fmtbuf, strerror(saved_errno));
> va_end(ap);
> }
>
> @@ -159,21 +146,20 @@ log_debug(const char *emsg, ...)
> static void
> vfatalc(int code, const char *emsg, va_list ap)
> {
> - static char s[BUFSIZ];
> const char *sep;
>
> if (emsg != NULL) {
> - (void)vsnprintf(s, sizeof(s), emsg, ap);
> + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> sep = ": ";
> } else {
> - s[0] = '\0';
> + fmtbuf[0] = '\0';
> sep = "";
> }
> if (code)
> logit(LOG_CRIT, "fatal in %s: %s%s%s",
> - log_procname, s, sep, strerror(code));
> + log_procname, fmtbuf, sep, strerror(code));
> else
> - logit(LOG_CRIT, "fatal in %s%s%s", log_procname, sep, s);
> + logit(LOG_CRIT, "fatal in %s%s%s", log_procname, sep, fmtbuf);
> }
>
> void
>
