[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [RESEND PATCH] add bb_info_msg was Re: Applets send errors to syslog during normal, successful o
From:       Tito <farmatito () tiscali ! it>
Date:       2018-02-28 20:13:12
Message-ID: 057cec31-4862-6921-75a7-974a631d8f92 () tiscali ! it
[Download RAW message or body]



On 28/02/2018 18:27, Deweloper wrote:
> On Wed, 28 Feb 2018 14:03:46 +0100
> Tito <farmatito@tiscali.it> wrote:
> 
> > Hi,
> > 
> > forgot to add the [PATCH] tag so I resend it.
> > 
> > Ciao,
> > Tito
> > 
> > On 04/02/2018 19:53, Denys Vlasenko wrote:
> > > On Thu, Nov 30, 2017 at 9:51 PM, Tito <farmatito@tiscali.it> wrote:
> > > > On 11/30/2017 08:26 PM, Deweloper wrote:
> > > > > Many applets are daemons (or can be run as daemons) and send messages to
> > > > > syslog. The problem is that the messages don't have accurate, individually
> > > > > assigned severity; they are all LOG_ERR. Effectively, system
> > > > > administrator sees a lot of ERRORs in the log even when everything goes
> > > > > well. It seems that libbb provides only bb_error_msg() as a convenient
> > > > > way to print a message (including sending it to syslog), while a more
> > > > > generic function taking severity as well would be needed instead. grep -r
> > > > > 'syslog(' shows that only some loginutils call syslog() directly. In
> > > > > other places bb_error_msg() is used even for informational or verbose
> > > > > debugging messages. Just have a look at output of grep -r 'bb_error_msg('
> > > > > 
> > > > > Do you have an idea how to clean this up? Shouldn't these messages be
> > > > > sent to a new function, e.g. bb_msg(), which would additionally take
> > > > > "severity" argument?
> > > 
> > > The "severity" arg usually ends up being a PITA.
> > > For example, it's rather unreadable.
> > > Then, it tends to proliferate: after you have errors/not-errors,
> > > then someone wants critical/errors/not-errors/debug - which adds
> > > another dimension to coding every error message: "what severity is it?!"
> > > 
> 
> Sorry, but this is how syslog() API is designed.
> 
> > > 
> > > > 4) create a function that changes syslog_level to LOG_INFO, add it to
> > > > libbb, and then change the code of the applets accordingly, for example:
> > > > 
> > > > void FAST_FUNC bb_info_msg(const char *s, ...)
> > > > {
> > > > va_list p;
> > > > #if ENABLE_FEATURE_SYSLOG
> > > > smallint syslog_level_old =  syslog_level;
> > > > syslog_level = LOG_INFO;
> > > > #endif
> > > > va_start(p, s);
> > > > bb_verror_msg(s, p, NULL);
> > > > #if ENABLE_FEATURE_SYSLOG
> > > > syslog_level = syslog_level_old;
> > > > #endif
> > > > va_end(p);
> > > > }
> > > 
> > > I like this. Feel free to send patches doing this.
> > > 
> 
> As of busybox 1.28.0 the exported global variable syslog_level seems to be used \
> only in one place @miscutils/crond.c:
> > syslog_level = LOG_INFO;
> > bb_verror_msg(msg, va, /* strerr: */ NULL);
> > syslog_level = LOG_ERR;
> IMO it would be much better to completely get rid of it and - surprise - use \
> function parameters for passing parameters to functions. 
Hi,
I think that it is not that practical to add one more parameter to all
bb_error_msg and bb_error_msg_and_die calls because:

1) they are error messaging functions and LOG_ERR is mostly right
2) if ENABLE_FEATURE_SYSLOG is not set they don't do any syslog
    messaging at all and the parameter would be superfluous.
3) it will increase space
4) syslog_level allows already to to handle exceptions (set to LOG_INFO
    or other LOG_...)
5) bb_info_msg is a commodity function of 4) to better tune
    syslog output by using existing machinery of libbb
    without "uglyfying" the code with syslog_level = LOG_INFO
    and syslog_level = LOG_ERR line pairs and it
    is automagically converted to bb_error_msg if
    ENABLE_FEATURE_SYSLOG is not set, needs no extra args and
    should not increase binary size that much.

Ciao,
Tito

BTW: miscutils/crond.c: seems the perfect place for using bb_info_msg.

_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox


[prev in list] [next in list] [prev in thread] [next in thread]