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

Wed May 14 13:26:55 CDT 2014

My proposed fix is attached: Log.cxx and Log.hxx (including the 
isEqualNoCase thing), changes based off of resiprocate 1.9.6.

-John Gregg

On 05/14/2014 01:33 PM, slgodin at gmail.com wrote:
> Hi John,
>
> I agree - fixing fragility is a good thing.  Can you please post a 
> patch file for these changes and I can investigate applying when I 
> have some time?  Please remember not to use strcasecmp as it is not 
> portable.
>
> Thanks for contributing!
>
> Scott
>
> On May 14, 2014, at 1:21 PM, John Gregg <jgregg at aylus.com 
> <mailto:jgregg at aylus.com>> wrote:
>
>>
>> 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/09a02b79/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Log.cxx
Type: text/x-c++src
Size: 25610 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20140514/09a02b79/attachment.cxx>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Log.hxx
Type: text/x-c++hdr
Size: 17687 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20140514/09a02b79/attachment.hxx>