I am doing a project where I use DLL's as run-time modules or plugins. To do this, I use the Windows LoadLibrary()
and FreeLibrary()
API calls to call two functions exported by the DLL. I decided to make a class so dealing with the DLL's would be easier. Is there anything I can add to my class to make it better, is there anything I'm doing wrong? Feel free to criticize anything you want.
SaModule.h
#ifndef SAMODULE_H__
#define SAMODULE_H__
#include <Windows.h>
#include <string>
#include <exception>
#include "WinException.h"
class SaModule
{
private:
typedef const char* (*LPFNGETNAME)();
typedef bool (*LPFNDOSCAN)(std::string&, std::string&);
HINSTANCE hInstance;
std::string path;
public:
SaModule();
SaModule(const std::string& path);
~SaModule();
void Load(const std::string& path);
void Unload();
LPFNDOSCAN DoScan;
LPFNGETNAME GetName;
HINSTANCE GetHandle() const;
operator HINSTANCE() const;
};
#endif//SAMODULE_H__
SaModule.cpp
#include "SaModule.h"
#include "WinException.h"
#include <sstream>
#include <iomanip>
SaModule::SaModule(): path(), hInstance(NULL)
{
//Nothing...
}
SaModule::SaModule(const std::string& path): path(path), hInstance(NULL)
{
Load(path);
}
SaModule::~SaModule()
{
Unload();
}
void SaModule::Load(const std::string& path)
{
this->path = path;
if( NULL == (hInstance=LoadLibrary(path.c_str())) )
throw WinException();
if( NULL == (GetName=reinterpret_cast<LPFNGETNAME>(GetProcAddress(hInstance,"GetName"))) )
throw WinException();
if( NULL == (DoScan=reinterpret_cast<LPFNDOSCAN>(GetProcAddress(hInstance, "DoScan"))) )
throw WinException();
}
void SaModule::Unload()
{
FreeLibrary(hInstance);
}
HINSTANCE SaModule::GetHandle() const
{
return hInstance;
}
SaModule::operator HINSTANCE() const
{
return hInstance;
}
WinException.h
#ifndef WINEXCEPTION_H__
#define WINEXCEPTION_H__
#include <Windows.h>
#include <exception>
#include <string>
class WinException: public std::exception
{
private:
DWORD dwError;
std::string errMsg;
public:
WinException();
const char* what() const;
};
#endif//WINEXCEPTION_H__
WinException.cpp
#include "WinException.h"
#include <sstream>
#include <iomanip>
WinException::WinException()
{
std::ostringstream ss;
dwError = GetLastError();
LPVOID lpMsgBuf = NULL;
FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM|FORMAT_MESSAGE_IGNORE_INSERTS,
NULL, dwError, MAKELANGID(LANG_NEUTRAL,SUBLANG_DEFAULT), reinterpret_cast<LPTSTR>(&lpMsgBuf), 0, NULL);
ss << "Error: 0x" << std::hex << std::setfill('0') << std::setw(8) << dwError << std::endl
<< reinterpret_cast<LPTSTR>(lpMsgBuf);
errMsg = ss.str();
LocalFree(lpMsgBuf);
}
const char* WinException::what() const
{
return errMsg.c_str();
}
Example Usage
#include <iostream>
#include "SaModule.h"
int main(int argc, char* argv[])
{
try
{
SaModule mod("some.sa.dll");
std::cout << "Loaded Module: " << mod.GetName() << std::endl;
}
catch( WinException& ex )
{
std::cout << ex.what() << std::endl;
}
return 0;
}
3 Answers 3
Just a couple of quick thoughts.
DoScan
andGetName
probably should be initialised to NULL as well.Load
doesn't callUnload
for any previous loaded dlls. If Load should be called once only perSaModule
, that should be detected.- A more informative exception could be thrown. "...during attempted insertion of plugin xyz", for example.
- Beware of function name decoration, if that could be an issue.
First thing that pops out at me:
if( NULL == (hInstance=LoadLibrary(path.c_str())) )
When programming for Windows, you should almost never want to use the "A
"/"ANSI" versions of functions. This is legacy from Win9x and you should be using Unicode. Windows is not like Unix where the C library can assume UTF-8 and everything can keep chugging along with the full set of Unicode chars accesible: by restricting yourself to the "multi-byte" encodings you will (1) see inconsistent behavior depending on what the user sets their language to and (2) not be able to access some Unicode filenames that the user may have on their filesystem. You should define the UNICODE
and _UNICODE
macros and use WCHAR
instead of 8-bit chars. (Since it seems you want to use the C++ classes I believe wstring
works instead of string
here.)
Second:
The WinException
class looks weird to me. Not all Windows APIs use GetLastError
- some of them may return HRESULT
or do something else. From that perspective you have not captured the full generality of Windows errors.
I would also say, perhaps controversially since this is not the path everybody has taken, that wrapping Win32 errors with exceptions is a bad idea. There are some Win32 errors that are not error conditions at all, for example FindNextFile
will fail with ERROR_NO_MORE_ITEMS
when you reach the end of a directory, or I/O will fail with ERROR_IO_PENDING
to indicate that the I/O is happening asynchronously. A Win32 program will occasionally have to react to such errors and I would say the way you've done it loses some of that information, or makes it less convenient to work with.
But maybe it's OK for your use. YMMV.
-
4\$\begingroup\$ Instead of roll-your-own
WinException
, should be usingsystem_error
and the Windows-specificsystem_category
of errors. The best article I've found about this is at Chris Kohlkoff's blog. \$\endgroup\$Mike C– Mike C2012年09月28日 20:20:34 +00:00Commented Sep 28, 2012 at 20:20 -
\$\begingroup\$ @MikeC - Interesting, didn't know about this addition to C++11. +1 \$\endgroup\$asveikau– asveikau2012年09月28日 20:31:36 +00:00Commented Sep 28, 2012 at 20:31
As MikeC suggested, the WinException class could be replaced by std::system_error
. This original came from the boost::system
library and was included into the C++0x specification with a few changes (mainly some of the names).
Example for throw
:
throw std::system_error(std::error_code(::GetLastError(),
std::system_category()), "SaModule::Load()");
Example for catch
:
//...
catch( std::system_error& se )
{
std::cout << "Error: " << se.code().value() << "\n";
std::cout << "Message: " << se.code().message() << "\n";
std::cout << "What: " << se.what() << "\n";
}
Example output for ERROR_FILE_NOT_FOUND
:
Error: 2
Message: No such file or directory
What: SaModule::Load()
SAMODULE_H__
cause the identifier to infringe upon the reserved ones. Even if you've seen this, read the double underscores part carefully: stackoverflow.com/questions/228783/… \$\endgroup\$