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

[Scott Godin]

Hi John,

I'm hesitating to commit this change, since it entails a performance hit
for every line logged.  The previous code printed the string version of the
log level using a fast array index.  This change iterates through the new
level array for every line logged.  While I like that it's less fragile,
I'm not sure the performance change can be justified .

Regards,
Scott


On Wed, May 14, 2014 at 2:26 PM, John Gregg <jgregg at aylus.com> wrote:

>
> 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> 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> 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
>> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20140520/83fca7ec/attachment.htm>