CodeGit Merge Request #7: Several improvements to NTEventLogAppender (merged)

The NTEventLogAppender can now be instantiated with a location of the NTEventLogAppender.dll. By specifying a full path to the file, it's not necessary to make sure the file is in the PATH.

Also added a constructor that doesn't initialize the registry and keeps the appender closed.

Also fixed a few minor bugs and improved internal consistency.

The changes should be fully backwards compatible, i.e. any existing code that uses the NTEventLogAppender as intended, should still work the same way.

Please see check-in comments in the branch for many more details.

[Alexander Perepelkin]

Hello Jac,

Thank you for this great amount of work and sorry that I haven't replied earlier.
I will seek through the changes and let you know if there are any additions I wish to be considered.

Alex.

[Alexander Perepelkin]

I've looked through the commits for changing NTLogAppender and got there few questions. Would you tell please:

• In your client code do you call reopen() explicitly in some cases?
• Did you try to compile the project both with Unicode turned on and off? I am asking that because Windows have pair of functions for API calls.
  E.g, call to RegSetValueEx() resolves to RegSetValueExA() (Ansi) or RegSetValueExW() (Unicode)
  It could be reasonable to use more wide type LPCTSTR instead of concrete LPCSTR.
  (quick link to that is https://stackoverflow.com/a/8481533/532541 )

[Jac Goudsmit]

Hi Alex,

In your client code do you call reopen() explicitly in some cases?

Whether I personally call reopen in my project is not important. But reopen is a public function so it CAN be called. If you're asking this to find out why I moved the registry code to an overloaded reopen(), the reasons for that are:

• The old open() didn't guard internal consistency: if a subclass would call open() twice in a row without calling close(), open() would leak a handle.
• To fix this, a call to close() could have been added to the open() function
• But with the open() function calling close first, it basically (semantically) becomes a reopen. There is already an override virtual function called reopen() so why not move the functionality there.
• Calling reopen on NTEventLogAppender may not make much sense because it's not like you're closing a file, deleting it and then reopening it or something. But in a derived class, someone might want to reimplement reopen() for a good reason. Perhaps they want to use reopen to delete all previously logged events for the logger, and then start logging again. Who knows.
• In the old version, reopen would always check the registry to make sure that at least the registry key is present. But in most use cases (at least if the program is running with sufficient access rights), the event logging starts right after construction and it's unnecessary to create the key and/or check if the registry key exists after construction, especially if it's impossible to change the source name anyway. So there's no reason to put the registry in the reopen() code if all the program wants to do is unregister and re-register the event source.
• The constructor of the old code always called open() to start logging. But as I mentioned in the bug report, open() is a virtual function and in C++, when a virtual function is called from a constructor, it always executes the implementation that belongs to the constructor, not the final class that's being constructed. That means that even if I write a subclass with an override for open(), NTEventLogAppender::open would still be called from the constructor, and would still create registry keys and values that my subclass may not want to create. For example, my subclass may not know yet what the source name is that a different part of my application wants to use for logging. Or it may want to use a different resource DLL. Or it may want to call ImpersonateLoggedOnUser to make sure that it can modify the registry from a non-elevated application. So I added a constructor that doesn't execute the registry code to make it possible to accomplish this. But that means of course that I also had to make it possible to call a function to make the registry changes at a later stage (after construction). For this, an overloaded virtual reopen() seemed like the best idea.

Did you try to compile the project both with Unicode turned on and off? I am asking that because Windows have pair of functions for API calls.

I don't know if the Log4CPP took any measures to #undef _UNICODE before #include <tchar.h> but if it doesn't, a project that has _UNICODE defined and includes log4cpp/NTEventLogAppender.h, will see an external declaration of (say) HKEY regGetKey(wchar_t *subkey, DWORD *disposition); because TCHAR * is interpreted as wchar_t * instead of char *. So when someone wants to write a derived class that calls this function, their compiler will complain if they want to call the function with e.g. "mysubkey" because that's not a wide-character string. So they'll use L"mysubkey" or TEXT("mysubkey~) which will compile without errors or warnings. But then the linker will look for a public symbol in any library (including the import library of log4cpp) for the function with the wchar_t pointer and it won't find it, because the log4cpp library was compiled without Unicode, so the mangled function name has a different symbol in the import library and the DLL than what the linker of this user is looking for.

Because Windows has an API that's mostly based on C and PASCAL, overloading between two functions with the same name but different parameters (char vs. wchar_t) is impossible. So Windows uses a preprocessor trick to do this: When you call CreafeFile you really call CreateFileA or CreateFileW depending on your Unicode setting.

The old NTEventLogAppender code only provides one implementation of the registry functions, so unless it takes measures to define its own TCHAR type, you're making the preprocessor and compiler promise something that isn't delivered by the library: a Unicode version of the functions. There are two ways to solve this: either provide two versions of the functions or change the declaration of the function into a non-ambiguous one. Since the registry functions aren't really a core functionality of the NTEventLogAppender anyway (they are arguably just made available as protected functions for convenience), I decided to do the latter: change the declaration so that it would be non-ambiguous. There's just one problem with my change: The change from char * to LPCSTR (which translated to const char *) will also change the mangled signature of the functions, so that those who use the registry functions will have to recompile and relink to the import library (if you simply replace the DLLs in an existing binary project, you'll get an execution error if the registry functions are used). But no code has to be changed in any client projects that use the registry functions. I figured this would be a small and acceptable price to pay.

Sorry this got so long. Thanks for reading and thanks for considering my changes!

===Jac

[Alexander Perepelkin]

Whether I personally call reopen in my project is not important. But reopen is a public function so it CAN be called. If you're asking this to find out why I moved the registry code to an overloaded reopen()...

I asked you whether you call it or not because you are the first user of this new code and may have a pattern.
Please put some thoughts into the following:

• you reasonably introduced new constructor so the dllLocation can be specified explicitly. After location of dll is specified for the process, it is hardly possible that the same process will call reopen(const std::string &sourceName, const std::string &dllLocation) and change any of parameters. Therefore dllLocation is preferably to become constant field of the class:

dllLocation.length() ? dllLocation.c_str() : "NTEventLogAppender.dll"

and reopen() be the only public overload for reopening

• calling virtual open() from constructor is indeed undesirable. Some private non-virtual openImpl() should be called instead. Calling reopen() from constructor is a bit more complicated than the reader expects although. Constructor is known to be the very first initializer. The reasonable expectation in ctr would be just openImpl() without re-opening.

I'll get back to TCHAR etc. a bit later.

[Jac Goudsmit]

You reasonably introduced new constructor so the dllLocation can be specified explicitly. After location of dll is specified for the process, it is hardly possible that the same process will call reopen(const std::string &sourceName, const std::string &dllLocation) and change any of parameters. Therefore dllLocation is preferably to become constant field of the class:

If I understand you correctly you want to store the DLL location at construction time, and then not allow the client code to change it anymore, so it's not possible to change the location.

I disagree with that. I think there are possible use cases where the Appender is created when the program is started, but where the DLL location is not yet known until later, or may change later. For one thing, the registry key where the subkey and values need to be stored, is read-only unless the program runs as administrator. So an application might do an impersonation to get sufficient access rights to the registry, and then initialize the registry values and keys.

calling virtual open() from constructor is indeed undesirable. Some private non-virtual openImpl() should be called instead. Calling reopen() from constructor is a bit more complicated than the reader expects although. Constructor is known to be the very first initializer. The reasonable expectation in ctr would be just openImpl() without re-opening.

If I understand you correctly, you would like to separate the registry code into a separate function that's private and gets called from the constructor as well as from reopen(), but keep the other code.

I see the advantage in that: the constructors would call that function instead of a virtual function (which is undesirable as we discussed because it might confuse people), and it wouldn't unnecessarily call close() (via reopen) as part of the construction.

I think that's a good idea but I would do it slightly differently. How about if I write a static public function NTEventLogAppender::InitRegistry that creates or updates a registry key for event logging, and takes parameters for all the 4 values that are stored in the registry, with default values that correspond to the current situation?

class NTEventLogAppender : public AppenderSkeleton {

...

public:
    /**

      * Initialize an event logging source in the registry
      * <br>
      * This is called as part of the constructors (except the non-opening
      * constructor) and as part of the reopen() function with parameters.
      * It initializes the registry to prepare it for logging events.
      * <br>
      * Note: the program must run in elevated mode for this to be successful; normal users don't have write-access to the part of the registry that's modified by this function.
      * <br>
      * If the DLL locations don't have a drive and directory, Windows will
      * search for them on the path.
      *
      * @param EventMessageFile Path to DLL for event messages, default "NTEventLogAppender.dll"
      * @param CategoryMessageFile Path to DLL for event categories, default "NTEventLogAppender.dll"
      * @param TypesSupported Number of event messages in DLL, should be (and defaults to) 7 for NTEventLogAppender.dll
      * @param CategoryCount Number of categories in DLL, should be (and defaults to) 8 for NTEventLogAppender.dll
      * @returns true if successful, false on error
      */
    static bool InitRegistry(
        const char *EventMessageFile = "NTEventLogAppender.dll",
        const char *CategoryMessageFile = "NTEventLogAppender.dll",
        DWORD TypesSupported = 7,
        DWORD CategoryCount = 8);


    ...


    NTEventLogAppender::NTEventLogAppender(const std::string& name, const std::string& sourceName) :
    AppenderSkeleton(name),
    _strSourceName(),
    _hEventSource(NULL)
    {
        InitRegistry();
    }


    bool NTEventLogAppender::reopen(const std::string &sourceName, const std::string &dllLocation) {
        close();

        // Copy the source name; don't proceed with open unless it's non-blank
        bool result = (0 != (_strSourceName = sourceName).GetLength());

        // If source name is given and DLL location is given, init registry
        if (result) {
            const char *s = dllLocation.c_str();

            result = *s ? InitRegistry(s, s) : true;
        }

        if (result) {
            result = reopen();
        }

        return result;
    }

===Jac

[Alexander Perepelkin]

1) Object of NTEventLogAppender class is initialized automatically using values suplied in config file.

src/PropertyConfiguratorImpl.cpp
appender = new NTEventLogAppender(appenderName, source);

It's not responsibility of the library to track changes of the configuration in registry. If something gets changed in configuration, logging subsystem may be shutdown and reinitialized by the directive from the application. This is why name, source and dllLocation must be immutable fields.

2) I see the public static method is a bit overkill for the task. The only need here is to move virtual method away from ctr, this is well solved by having separate private impl method. public static would be visible outside and confuse potential users of the class.

[Jac Goudsmit]

I'm sorry that it's taking me a while to get back on this; I've been very busy with work and other projects. I hope to fix the code in my repo later this week, then I'll update this discussion. Thanks!

===Jac

[Jac Goudsmit]

Hi Alex,

I just changed the code so that the member variables of the NTEventLogAppender class are not changed after construction.

That means some things that were possible before this latest change (19cf8a969762599a198e0f276238d4b86977ffc9) like changing the DLL location on the fly, aren't possible anymore (as you requested), but I made it so that they can still be accomplished by creating a subclass.

Thanks for considering!

===Jac

[Alexander Perepelkin]

Summing comments on commits in merge request and my replies:

• close() now resets _hEventSource to NULL to prevent dangling handle

close() is either the last action performed by the class in destructor, or the action which is immediately followed by the open() and rewites handle right away. Hence, though nulling it might look like a proper precaution, it is redundant.

• Functions that use _hEventSource now check for the handle to be non-NULL before using it (prevents invalid parameter exceptions in recent versions of Windows)
• Added a check for empty _strSourceName to open()

Having checks in functions open() and _append() would hide mistaken parameters instead of raising errors.
If _strSourceName is empty, it most probably means that this is a mistake and it should be caught and fixed during testing (instead of being hidden behind a check)

The old open() function was only safe to call when the appender is in closed state, otherwise it would leak a handle.

open() is protected and can not be called by clients of the class. Other calls are controlled by the implementation and those do not leak a handle.

Therefore it made sense to move the functionality of the open() function to the reopen() function <...>

That would mix semantics of open/reopen, making implementation more complex than it can be.

The old version of addRegistryInfo would always use "NTEventLogAppender.dll" without drive or directory, so it was necessary to put the DLL in the PATH or change the PATH to add the directory where the DLL is stored
Finally, the protected registry functions used TCHAR in their prototypes, which is a very bad idea: programs that are compiled in Unicode and use these functions would be incompatible with the log4cpp.dll because the DLL and the program would disagree on what a TCHAR is. This results in linker problems that are potentially very difficult or impossible to debug, especially for beginning Windows programmers.
Another problem was that the reopen() function would always get the registry key to check if it existed, which should not be necessary in most cases. And if the registry key already existed, the function wouldn't save the values so it was impossible to deliberately change the values.

These are all good points and could be fixed independently. Unfortunately, current merge request takes care of all of them together and they are now on top of another changes.
Rewriting registry keys on each reopen() call is solved by moving addRegistryInfo() invokation into constructor. That way multiple reopen() calls will not cause many cycles of equivalent writes to registry.
Other two points can be considered as a list of good items to do.