[PATCH] new bb_msg_and_syslog + sulogin clean up.

Rob Landley rob at landley.net
Fri Aug 25 17:38:13 UTC 2006


On Friday 25 August 2006 8:57 am, Tito wrote:
> > I think this would be conceptually cleaner as three functions:
> > 
> > write_syslog() - just does the syslog
> > perror_syslog() - syslog and stderr
> > perror_syslog_and_die() - syslog and stderr, then die.
> 
> Hi,
> Yes I've thought about this too, but my personal taste is
> to prefer one flexible function that can be used in the most common
> cases by changing the behaviour through some kind of flags,
> like in this case trough the level (a parameter we have to pass to the 
function
> anyway)

This week's been crazy, but this weekend I need to get this and Denis' new 
not-top code resolved.

> Feel free to change it ....or let me know how exactly it should work and i
> could try to reimplement it.

Part of what I need to do is read the syslog man pages more closely.

> BTW: i was thinking to eliminate the bb_facility global var by passing it
> also as part of the first parameter to msg_and_syslog:

According to man 3 syslog, the facility's optional anyway (it's ored with the 
level on each call to syslog) and the call to openlog is optional (it's only 
_needed_ to set ident.  And don't we want this to be bb_applet_name or some 
such?

Oh, and I think you're right that closelog() is only needed for 
feature_clean_up because I think what it's really doing is opening a 
filehandle and saving it in a static variable, and re-using it.  (I don't 
think multiple calls to openlog will leak filehandles.  Something to test...)

By the way, if we use LOG_PERROR then we dont' need the msg_and_log parts.  
Depends on whether we want to trust another glibc extension. :)

Do we want to configure globally whether or not we want applets to log stuff?

> Optionally also a LOG_DIE flag could be added to make it possible
> to define with a better granularity when we want the app to die or not.... 

*shrug*

> > In theory each lower one is a wrapper around a higher one, but I'm not 
quite 
> > sure how to do the va_stuff handoff to get them both to work like printf.
> 
> Maybe something like this?!

It turns out there's a vsyslog(), which makes things easier...
 
> > Any debug_syslog() thing should be a macro that optimizes out if 
ENABLE_DEBUG 
> > is 0.  I don't think that's really required, though.
> 
> Yes, it is just there to rip out the check for       LOG_DEBUG
> as it is unlikely to be found when ENABLE DEBUG is not set
> 
> USE_DEBUG(case LOG_DEBUG:)

I meant optimize away the caller, not our processing of the message.  Save the 
function call and the string.

But I'm really not a big fan of debug messages anyway.  They never mean as 
much to other people as they do to the original developer, and instrumenting 
with dprintfs to binary search your way to the problem is pretty standard 
procedure...

> > Closing the fd only if FEATURE_CLEAN_UP is set strikes me as dangerous.  
What 
> > if dnsd or httpd starts using this?  The leaks can add up and get nasty 
after 
> > a while.
> 
> Just followed the man:
> closelog() closes the descriptor being used to write to the system logger.  
The use of closelog() is optional.
> Will fix it.

The man page suggests to me that closelog() is indeed optional, so 
FEATURE_CLEAN_UP makes sense here.  (Well, since it's a re-used static 
filehandle to a global system service it's actually pretty deeply useless to 
close it, even for the nofork guys like Shaun Jackman, but the people who 
want to use strange memory debugging libraries complain about this sort of 
thing...)

> > Hmmm...  I've been pondering consolidating show_usage(), error_msg(), 
> > error_msg_and_die(), perror_msg(), perror_msg_and_die(), vherror_msg(), 
> > herror_msg(), herror_msg_and_die(), perror_nomsg_and_die(), 
perror_nomsg(), 
> > verror_msg(), and vperror_msg().  They should all be in one .c file (ala 
> > llist.c or xfuncs.c).  This belongs in with that pile, although I won't be 
> > able to do that consolidation before this weekend.
> > 
> > I'm also likely to remove the bb_ prefixes at the same time because I 
really 
> > don't like them. 
> 
> Could rename it to msg_and_syslog.

Woot.

> > The documentation says LOG_QUIET but the code says LOG_ONLY...
> 
> Yes have to fix it to LOG_ONLY as in libbb.h.

As long as it's consistent. :)

Ok, marking the message I just replied to as TODO...

Rob
-- 
Never bet against the cheap plastic solution.



More information about the busybox mailing list