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

[John Gregg <jgregg@xxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel