Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.10.0
    • Fix Version/s: 0.10.0
    • Component/s: Layout
    • Labels:
      None
    • Environment:
      Kubuntu Linux 7.04, kernel 2.6.22-14-generic #1 SMP i686 GNU/Linux
      g++ (GCC) 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)

      Description

      htmllayout.cpp (line 128) has a bug when checking if ndc is null. Instead of:
      if(event->getNDC().length() != 0)
      it should be:
      if(!NDC::isNull(event->getNDC()))

      While using the HTMLLayout it's easily detected as 'NDC: null' appears.
      I've just downloaded the last svn version, so I think the problem hasn't been solved jet.
      Replacing line 128 and adding the ndc header:
      #include <log4cxx/ndc.h>
      are the only actiones needed to fix this bug; I've tested it and it works correctly – thou I only tested it without ndc.

        Activity

        Hide
        Akadafukah added a comment -

        Probably other layout are affected by a similar problem. However, the solution depends on the design criteria of the library developers. For instance, the PatternLayout has the same issue: the 'ndcpatternconverter.cpp' does the following:
        toAppendTo.append(event->getNDC());
        Should it check if the 'LogString' returned 'isNull', so we'll have this (non-optimized) version:
        if(!NDC::isNull(event->getNDC())) toAppendTo.append(event->getNDC());

        With the current version, if the parameter '%x' is given, we've the text 'null' written; IMHO it seems meaningless. I guess you're working on it (changing NDC) and you'd not treated jet, or maybe you'd chosen a most verbose alternative, but in the case of HTMLLayout is not configurable and so verbose (when NDC is null).

        bye

        Show
        Akadafukah added a comment - Probably other layout are affected by a similar problem. However, the solution depends on the design criteria of the library developers. For instance, the PatternLayout has the same issue: the 'ndcpatternconverter.cpp' does the following: toAppendTo.append(event->getNDC()); Should it check if the 'LogString' returned 'isNull', so we'll have this (non-optimized) version: if(!NDC::isNull(event->getNDC())) toAppendTo.append(event->getNDC()); With the current version, if the parameter '%x' is given, we've the text 'null' written; IMHO it seems meaningless. I guess you're working on it (changing NDC) and you'd not treated jet, or maybe you'd chosen a most verbose alternative, but in the case of HTMLLayout is not configurable and so verbose (when NDC is null). bye
        Hide
        Curt Arnold added a comment -

        In rev 601521, I changed MDC and NDC so that you can unambiguously determine whether the NDC is unset or if the MDC has a value for a specific key. I also removed some commented or ifdef'd out code and declarations. Attempting to use a special value to represent unset would continue to be problematic.

        %x in PatternLayout will output "null" if the NDC is unset since that mimics log4j's behavior. log4j calls StringBuffer.append with the returned value from event->getNDC() without checking for null and StringBuffer just replaces that with "null". However %X

        {foo}

        does check for null first. Not internally consistent, but log4cxx does appear to mimic log4j at least.

        Thanks for the report. Would appreciate testing or any feedback on the changes.

        Show
        Curt Arnold added a comment - In rev 601521, I changed MDC and NDC so that you can unambiguously determine whether the NDC is unset or if the MDC has a value for a specific key. I also removed some commented or ifdef'd out code and declarations. Attempting to use a special value to represent unset would continue to be problematic. %x in PatternLayout will output "null" if the NDC is unset since that mimics log4j's behavior. log4j calls StringBuffer.append with the returned value from event->getNDC() without checking for null and StringBuffer just replaces that with "null". However %X {foo} does check for null first. Not internally consistent, but log4cxx does appear to mimic log4j at least. Thanks for the report. Would appreciate testing or any feedback on the changes.

          People

          • Assignee:
            Curt Arnold
            Reporter:
            Akadafukah
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development