Theo Buehler([email protected]) on 2023.10.17 09:13:15 +0200:
> 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.
as far as i can remember, it was done so it would still be able to work
somewhat when out of memeory.
> > 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));
can you turn this around (if (debug) ...)
> > 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.
yes please :)
>
> > - /* 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
> >
>
