[reSIProcate] Log::initialize should take log level in upper or lower case (rutil)

[John Gregg]

Great. Thanks.

While I'm at it, it seems to me that there is a not entirely obvious 
dependency between mDescriptions[] in Log.cxx and Log::Level in Log.hxx. 
That is, the position of the strings in mDescriptions[] absolutely must 
correspond exactly with the values defined in the Log::Level enum. 
Moreover, Log::Level is defined with an #ifdef in the middle of it, and 
with various hardcoded defines in it (stuff set equal to other symbols 
from linux header files pulled in from elsewhere), and it all ends up 
compiling just right to match up with mDescriptions[], both in linux and 
Windows. So it works, but it is fragile in non-obvious ways.

The quick and dirty option would be to at least warn people with 
comments. That is, a comment in Log.cxx saying "NOTE: This list is 
order-dependent and MUST be kept in sync with the values defined in 
Log::Level", and a corresponding comment in Log.hxx saying "NOTE: The 
values defined here MUST be in sync with the order of the strings in 
mDescriptions[]".

Even better would be to get rid of the dependence as outlined below.

In Log.hxx, within class Log:

     struct descriptionEntry
     {
         Level level;
         const char * description;
     };

...

       static const descriptionEntry mDescriptions[];

Then, in Log.cxx:

const struct Log::descriptionEntry Log::mDescriptions[] =
{
     { None, "NONE" },
     { Crit, "CRIT" },
     { Crit, "CRITICAL" },
     { Err, "ERR" },
     { Err, "ERROR" },
     { Warning, "WARNING" },
     { Warning, "WARN" },
     { Info, "INFO" },
     { Debug, "DEBUG" },
     { Stack, "STACK" },
     { StdErr, "CERR" },
     { Bogus, NULL }
};

...

Data
Log::toString(Level l)
{
     int i;

     for (i = 0; mDescriptions[i].description; i++)
     {
         if (mDescriptions[i].level == l)
         {
             return log_ + mDescriptions[i].description;
         }
     }
     return log_ + "INVALID";
}

Log::Level
Log::toLevel(const Data& l)
{
    Data pri( l.prefix("LOG_") ? l.substr(4) : l);

    int i = 0;

    for (i = 0; mDescriptions[i].description; i++)
     {
         if (strcasecmp(pri.c_str(), mDescriptions[i].description) == 0)
       {
           return mDescriptions[i].level;
       }

    }

    cerr << "Choosing Debug level since string was not understood: " << 
l << endl;
    return Log::Debug;
}

Note that this includes the strcmp => strcasecmp change, and note also 
that it maps several strings ("warn", "warning") to the same error 
level, but while converting from a level back to a string, it will grab 
the first one it hits, so the preferred canonical string should be first 
in the struct (hence the "WARNING" entry appears before "WARN").

-John Gregg


On 05/14/2014 11:29 AM, Scott Godin wrote:
> I can't see a downside as well.  Feel free to post "patch" files to 
> this list for inclusion in the project.  This one is very simple 
> though - no need for a patch file.  I can apply it.
>
> Note:  strcasecmp is not available on all platforms - I will use resip 
> internal isEqualNoCase instead.
>
> Thanks!
> Scott
>
>
> On Wed, May 14, 2014 at 11:17 AM, John Gregg <jgregg at aylus.com 
> <mailto:jgregg at aylus.com>> wrote:
>
>
>     In rutil/Log.cxx:
>
>     When application code initializes the logging system in
>     resiprocate, it passes the log level (sensitivity) to
>     Log::initialize() as a string. This string is translated to an
>     internal Log::Level enum by Log::toLevel(). Log::toLevel() is
>     smart about stripping a preceding "LOG_" (if present), but it
>     demands that the string be upper case (it uses a straight strcmp()
>     to compare with hardcoded strings in mDescriptions[]). People may
>     want, e.g., to use strings from prioritynames[] in syslog.h under
>     linux, which are lower-case. It seems we should accept "crit" just
>     as much as "CRIT". This would be a simple change of strcmp =>
>     strcasecmp in Log::toLevel(). I don't see a downside.
>
>     I do not have permission to modify resiprocate code myself. Is
>     this the proper way to feed suggestions into the community?
>
>     -John Gregg
>
>     _______________________________________________
>     resiprocate-devel mailing list
>     resiprocate-devel at resiprocate.org
>     <mailto:resiprocate-devel at resiprocate.org>
>     https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20140514/4de363fc/attachment.htm>