Re: [Patch] Improve logging function

Werner,

You are absolutely right, thanks for pointing this out.
I have fixed it - I hope for good this time.

Patch attached.

Regards,
Jacek


2013/10/4 Werner Coetzee <[email protected]>

>  <http://smtp05.clickatell.com/rs/CgHB8J>
>
>  *Werner **Coetzee* - Senior C Developer
>
> *E:* [email protected]
> *T:* 27 21 910 7700 *   F:* 27 21 910 7701
>
>     This e-mail message and all attachments transmitted with it are
> confidential and are intended solely for the addressee(s). If the reader of
> this message is not the intended recipient, you are hereby notified that
> any reading, dissemination, distribution, copying, or other use of this
> message or its attachment(s) is strictly prohibited. Clickatell does not
> recognise that a contract can be concluded by way of electronic
> communication. All contracts must be signed on paper and only authorised
> signatories in terms of the Clickatell Authorised Signatory Policy may
> contract on behalf of Clickatell. All provisions to the contrary contained
> in the Electronic Communications and Transactions Act 25 of 2002 are hereby
> specifically excluded.
>
> Hi****
>
> ** **
>
> I believe there is one downside to this implementation though.****
>
> ** **
>
> If you're running with log level and output level (for stderr) the same,
> the formatting will be done twice if I'm not mistaken.****
>
> Or if you have multiple log files the formatting will be done multiple
> times (but only in the FUNCTION_GUTS macro since it loops through all logs).
> ****
>
> ** **
>
> Simple enough to fix though.****
>
> ** **
>
> Regards****
>
> Werner****
>
> ** **
>
> ** **
>
> *From:* devel [mailto:[email protected]] *On Behalf Of *Jacek
> Raczkiewicz
> *Sent:* 04 October 2013 09:14
> *To:* Alexander Malysh
> *Cc:* [email protected]
> *Subject:* Re: [Patch] Improve logging function****
>
> ** **
>
> Hi, ****
>
> ** **
>
> Ok I have fixed my patch and attached in the email.****
>
> I miss-understood the locking here, mea culpa.****
>
> ** **
>
> Jacek****
>
> ** **
>
> 2013/10/3 Alexander Malysh <[email protected]>****
>
> Hi,
>
> for improvement +1 for the implementation -1. You change a way how we used
> rwlock . Rwlock is to protect
> log file list and not the output. Please keep rwlock in place then +1 from
> me.
>
> Alex
>
> Am 02.10.2013 um 17:52 schrieb Jacek Raczkiewicz <
> [email protected]>:****
>
>
> > Hi,
> >
> > I have been working on imrpving application that uses a lot libraries
> from kannel/gwlib.
> > I noticed that logging function does format the string even if there is
> no intention to save it to the log due to configured log level.
> > My improvement is that I moved the format() function inside the
> conditional block which check for log level.
> > This really make a difference when logged string is large (in our case
> it may be as big as 1MB) and that is formatted for no reason.
> >
> > Please include this fix into kannel codebase.
> >
> > Regards
> > Jacek Raczkiewicz****
>
> > <logging.diff>****
>
> ** **
>
>
>

*** kannel_fromSVN/gwlib/log.c	2013-10-02 11:02:28.288291416 -0400
--- kannel/gwlib/log.c	2013-10-07 12:29:27.383138476 -0400
***************
*** 508,522 ****
  #define FUNCTION_GUTS(level, place) \
  	do { \
  	    int i; \
  	    char buf[FORMAT_SIZE]; \
  	    va_list args; \
  	    \
- 	    format(buf, level, place, err, fmt, 1); \
              gw_rwlock_rdlock(&rwlock); \
  	    for (i = 0; i < num_logfiles; ++i) { \
  		if (logfiles[i].exclusive == GW_NON_EXCL && \
                      level >= logfiles[i].minimum_output_level && \
                      logfiles[i].file != NULL) { \
  		        va_start(args, fmt); \
  		        output(logfiles[i].file, buf, args); \
  		        va_end(args); \
--- 510,528 ----
  #define FUNCTION_GUTS(level, place) \
  	do { \
  	    int i; \
+ 	    int formatted = 0; \
  	    char buf[FORMAT_SIZE]; \
  	    va_list args; \
  	    \
              gw_rwlock_rdlock(&rwlock); \
  	    for (i = 0; i < num_logfiles; ++i) { \
  		if (logfiles[i].exclusive == GW_NON_EXCL && \
                      level >= logfiles[i].minimum_output_level && \
                      logfiles[i].file != NULL) { \
+                 	if (!formatted) { \
+                 	    format(buf, level, place, err, fmt, 1); \
+                 	    formatted = 1; \
+                 	} \
  		        va_start(args, fmt); \
  		        output(logfiles[i].file, buf, args); \
  		        va_end(args); \
***************
*** 536,546 ****
  	    char buf[FORMAT_SIZE]; \
  	    va_list args; \
  	    \
- 	    format(buf, level, place, err, fmt, 1); \
              gw_rwlock_rdlock(&rwlock); \
              if (logfiles[e].exclusive == GW_EXCL && \
                  level >= logfiles[e].minimum_output_level && \
                  logfiles[e].file != NULL) { \
                  va_start(args, fmt); \
                  output(logfiles[e].file, buf, args); \
                  va_end(args); \
--- 542,552 ----
  	    char buf[FORMAT_SIZE]; \
  	    va_list args; \
  	    \
              gw_rwlock_rdlock(&rwlock); \
              if (logfiles[e].exclusive == GW_EXCL && \
                  level >= logfiles[e].minimum_output_level && \
                  logfiles[e].file != NULL) { \
+                 format(buf, level, place, err, fmt, 1); \
                  va_start(args, fmt); \
                  output(logfiles[e].file, buf, args); \
                  va_end(args); \