Thanks for the review. Some answers below...
On Tue, Feb 25, 2014 at 6:14 PM, Assaf Gordon <[email protected]> wrote: > Hello Rainer, > > > On 02/25/2014 09:52 AM, Rainer Gerhards wrote: > >> >> I am looking for some code reviewers. >> >> I have worked hard on liblogging-stdlog, which aims at becoming the new >> enhanced syslog() API call. The library is thread- and signal-safe and >> offers support for multiple log drivers, just like log4j does. >> >> > Few things I think about from reading the code (haven't compiled or tried > it): > > 1. stdlog_init()/stdlog_deinit() use static variables, and hence not > really thread-safe. > I should probably add a comment. These are only set in stdlog_init(), which is flagged as non-thread safe. We need to have a valid starting point. > > 2. The internal message building functions (e.g. build_file_line, > build_syslog_frame) do not check the length of the buffer when adding > characters, e.g.: > --- > frame[i++] = ':'; > frame[i++] = ' '; > --- > A maliciously constructed message (which consumes all the way up to > __STDLOG_MSGBUF_SIZE (4096 bytes)) will cause a stack buffer overflow. > > good catch! Tracking this now here: https://github.com/rsyslog/liblogging/issues/8 3. Your new interface for "stdlog_log()" returns an "int" indicating > success or failure. > Compared to the standard "syslog(3)" interface which returns void and > never fails. > well... syslog() can fail, but will not tell you about this. > This puts more burden on the developer, and even makes the "abstraction" > futile. > For example, the "file:" driver returns "EAGAIN" if the write was partial. > Does this mean that a developer using "stdlib-logging" now needs to check > if the program is writing to a file or not (which is changeable in runtime > with ENV), and add extra code to write the log message again? also - it's > impossible to know how much of the log message was written or not... so > resuming isn't possible. > > I think this issue needs to be re-considered for stdlog-logging to be > useful and robust. > > All of these are good points, and I would appreciate more discussion on the topic. Basically, I have three choices: a) fail silently b) re-try until success c) return error I think a) is not a good option, as those developers that want to go the extra mile to handle errors have no chance with it. The problem with b) is that this can effectively block application code AND makes it impossible to write signal-safe code. Thus, I opted for c). > 4. The posix syslog(3) ( http://pubs.opengroup.org/onlinepubs/009695399/ > functions/closelog.html ) adds additional flag: "%m" turns into > "strerror(errno)". > This feature is lost with "stdlog_log()" > > Yes ... is this extremely important? I dropped it in favor of signal-safeness. If the overall consensus is that it is a must-have, I can see that I fit it in in non-signal-safe mode. > HTH, > Definitely! Thanks a lot. Looking forward to further feedback. Rainer > -gordon > > > _______________________________________________ > rsyslog mailing list > http://lists.adiscon.net/mailman/listinfo/rsyslog > http://www.rsyslog.com/professional-services/ > What's up with rsyslog? Follow https://twitter.com/rgerhards > NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad > of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you > DON'T LIKE THAT. > _______________________________________________ rsyslog mailing list http://lists.adiscon.net/mailman/listinfo/rsyslog http://www.rsyslog.com/professional-services/ What's up with rsyslog? Follow https://twitter.com/rgerhards NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you DON'T LIKE THAT.
