From 13049e5fc3004360df0c445c1537bfdda83f53c2 Mon Sep 17 00:00:00 2001 From: Alexey Sokolov Date: Mon, 19 Dec 2016 16:47:29 +0000 Subject: [PATCH] Refactor the way how modules are loaded. Make version checks more strict. This finishes attempt to preserve ABI between patch versions. That didn't work well, and the people who could make it work, left the project already. Close #1255 Close #1274 Close #172 --- include/znc/Modules.h | 70 +++++++++++++++++++++++++++++-------------- include/znc/version.h | 41 +++++++++++++++++++++++-- make-tarball.sh | 2 ++ src/Modules.cpp | 67 +++++++++++++++++++++-------------------- src/znc.cpp | 40 ++++--------------------- 5 files changed, 129 insertions(+), 91 deletions(-) diff --git a/include/znc/Modules.h b/include/znc/Modules.h index 063d0040..6efd5300 100644 --- a/include/znc/Modules.h +++ b/include/znc/Modules.h @@ -60,25 +60,53 @@ class CModInfo; #define ZNC_EXPORT_LIB_EXPORT #endif -#define MODCOMMONDEFS(CLASS, DESCRIPTION, TYPE) \ - extern "C" { \ - ZNC_EXPORT_LIB_EXPORT bool ZNCModInfo(double dCoreVersion, \ - CModInfo& Info); \ - ZNC_EXPORT_LIB_EXPORT bool ZNCModInfo(double dCoreVersion, \ - CModInfo& Info) { \ - if (dCoreVersion != VERSION) return false; \ - auto t_s = [&](const CString& sEnglish, \ - const CString& sContext = "") { \ - return sEnglish.empty() ? "" : Info.t_s(sEnglish, sContext); \ - }; \ - t_s(CString()); /* Don't warn about unused t_s */ \ - Info.SetDescription(DESCRIPTION); \ - Info.SetDefaultType(TYPE); \ - Info.AddType(TYPE); \ - Info.SetLoader(TModLoad); \ - TModInfo(Info); \ - return true; \ - } \ +/** C-style entry point to the module. + * + * First, core compares C strings with version and compilation options of core + * and module. If they match, assume that C++ classes have the same layout and + * proceed to filling CModInfo. + * + * Most parts of version-extra is explicitly not compared, otherwise all + * modules need to be rebuilt for every commit, which is more cumbersome for + * ZNC developers. However, the part set by user (e.g. +deb1), is compared. + * + * If this struct ever changes, the first field (pcVersion) must stay the same. + * Otherwise, name of ZNCModuleEntry function must also change. Other fields + * can change at will. + * + * Modules shouldn't care about this struct, it's all managed by ...MODULEDEFS + * macro. + */ +struct CModuleEntry { + const char* pcVersion; + const char* pcVersionExtra; + const char* pcCompileOptions; + void (*fpFillModInfo)(CModInfo&); +}; + +#define MODCOMMONDEFS(CLASS, DESCRIPTION, TYPE) \ + static void FillModInfo(CModInfo& Info) { \ + auto t_s = [&](const CString& sEnglish, \ + const CString& sContext = "") { \ + return sEnglish.empty() ? "" : Info.t_s(sEnglish, sContext); \ + }; \ + t_s(CString()); /* Don't warn about unused t_s */ \ + Info.SetDescription(DESCRIPTION); \ + Info.SetDefaultType(TYPE); \ + Info.AddType(TYPE); \ + Info.SetLoader(TModLoad); \ + TModInfo(Info); \ + } \ + extern "C" { \ + /* A global variable leads to ODR violation when several modules are \ + * loaded. But a static variable inside a function works. */ \ + ZNC_EXPORT_LIB_EXPORT const CModuleEntry* ZNCModuleEntry(); \ + ZNC_EXPORT_LIB_EXPORT const CModuleEntry* ZNCModuleEntry() { \ + static const CModuleEntry ThisModule = {VERSION_STR, VERSION_EXTRA, \ + ZNC_COMPILE_OPTIONS_STRING, \ + FillModInfo}; \ + return &ThisModule; \ + } \ } /** Instead of writing a constructor, you should call this macro. It accepts all @@ -1016,7 +1044,6 @@ class CModule { virtual EModRet OnSendToIRC(CString& sLine); ModHandle GetDLL() { return m_pDLL; } - static double GetCoreVersion() { return VERSION; } /** This function sends a given raw IRC line to the IRC server, if we * are connected to one. Else this line is discarded. @@ -1552,8 +1579,7 @@ class CModules : public std::vector { private: static ModHandle OpenModule(const CString& sModule, const CString& sModPath, - bool& bVersionMismatch, CModInfo& Info, - CString& sRetMsg); + CModInfo& Info, CString& sRetMsg); protected: CUser* m_pUser; diff --git a/include/znc/version.h b/include/znc/version.h index d0490162..f7d40e47 100644 --- a/include/znc/version.h +++ b/include/znc/version.h @@ -6,11 +6,11 @@ #define VERSION_MAJOR 1 #define VERSION_MINOR 7 #define VERSION_PATCH -1 -// This one is for display purpose +// This one is for display purpose and to check ABI compatibility of modules #define VERSION_STR "1.7.x" #endif -// This one is for ZNCModInfo +// Don't use this one #define VERSION (VERSION_MAJOR + VERSION_MINOR / 10.0) // You can add -DVERSION_EXTRA="stuff" to your CXXFLAGS! @@ -19,4 +19,41 @@ #endif extern const char* ZNC_VERSION_EXTRA; +// Compilation options which affect ABI + +#ifdef HAVE_IPV6 +#define ZNC_VERSION_TEXT_IPV6 "yes" +#else +#define ZNC_VERSION_TEXT_IPV6 "no" +#endif + +#ifdef HAVE_LIBSSL +#define ZNC_VERSION_TEXT_SSL "yes" +#else +#define ZNC_VERSION_TEXT_SSL "no" +#endif + +#ifdef HAVE_THREADED_DNS +#define ZNC_VERSION_TEXT_DNS "threads" +#else +#define ZNC_VERSION_TEXT_DNS "blocking" +#endif + +#ifdef HAVE_ICU +#define ZNC_VERSION_TEXT_ICU "yes" +#else +#define ZNC_VERSION_TEXT_ICU "no" +#endif + +#ifdef HAVE_I18N +#define ZNC_VERSION_TEXT_I18N "yes" +#else +#define ZNC_VERSION_TEXT_I18N "no" +#endif + +#define ZNC_COMPILE_OPTIONS_STRING \ + "IPv6: " ZNC_VERSION_TEXT_IPV6 ", SSL: " ZNC_VERSION_TEXT_SSL \ + ", DNS: " ZNC_VERSION_TEXT_DNS ", charset: " ZNC_VERSION_TEXT_ICU \ + ", i18n: " ZNC_VERSION_TEXT_I18N + #endif // !ZNC_VERSION_H diff --git a/make-tarball.sh b/make-tarball.sh index 2c6fdbac..f0a3f824 100755 --- a/make-tarball.sh +++ b/make-tarball.sh @@ -53,9 +53,11 @@ cp -p third_party/Csocket/Csocket.cc third_party/Csocket/Csocket.h $TMPDIR/$ZNCD rm -r autom4te.cache/ rm .travis* .appveyor* rm make-tarball.sh + # For autoconf sed -e "s/THIS_IS_NOT_TARBALL//" -i Makefile.in echo '#include ' > src/version.cpp echo "const char* ZNC_VERSION_EXTRA = VERSION_EXTRA \"$DESC\";" >> src/version.cpp + # For cmake if [ "x$DESC" != "x" ]; then echo $DESC > .nightly fi diff --git a/src/Modules.cpp b/src/Modules.cpp index ecd8fb8d..9f05eda2 100644 --- a/src/Modules.cpp +++ b/src/Modules.cpp @@ -1619,7 +1619,6 @@ bool CModules::LoadModule(const CString& sModule, const CString& sArgs, if (bHandled) return bSuccess; CString sModPath, sDataPath; - bool bVersionMismatch; CModInfo Info; if (!FindModPath(sModule, sModPath, sDataPath)) { @@ -1629,17 +1628,10 @@ bool CModules::LoadModule(const CString& sModule, const CString& sArgs, Info.SetName(sModule); Info.SetPath(sModPath); - ModHandle p = - OpenModule(sModule, sModPath, bVersionMismatch, Info, sRetMsg); + ModHandle p = OpenModule(sModule, sModPath, Info, sRetMsg); if (!p) return false; - if (bVersionMismatch) { - dlclose(p); - sRetMsg = "Version mismatch, recompile this module."; - return false; - } - if (!Info.SupportsType(eType)) { dlclose(p); sRetMsg = "Module [" + sModule + "] does not support module type [" + @@ -1785,20 +1777,12 @@ bool CModules::GetModInfo(CModInfo& ModInfo, const CString& sModule, bool CModules::GetModPathInfo(CModInfo& ModInfo, const CString& sModule, const CString& sModPath, CString& sRetMsg) { - bool bVersionMismatch; - ModInfo.SetName(sModule); ModInfo.SetPath(sModPath); - ModHandle p = - OpenModule(sModule, sModPath, bVersionMismatch, ModInfo, sRetMsg); + ModHandle p = OpenModule(sModule, sModPath, ModInfo, sRetMsg); if (!p) return false; - if (bVersionMismatch) { - ModInfo.SetDescription( - "--- Version mismatch, recompile this module. ---"); - } - dlclose(p); return true; @@ -1901,10 +1885,8 @@ CModules::ModDirList CModules::GetModDirs() { } ModHandle CModules::OpenModule(const CString& sModule, const CString& sModPath, - bool& bVersionMismatch, CModInfo& Info, - CString& sRetMsg) { + CModInfo& Info, CString& sRetMsg) { // Some sane defaults in case anything errors out below - bVersionMismatch = false; sRetMsg.clear(); for (unsigned int a = 0; a < sModule.length(); a++) { @@ -1942,24 +1924,43 @@ ModHandle CModules::OpenModule(const CString& sModule, const CString& sModPath, return nullptr; } - CTranslationDomainRefHolder translation("znc-" + sModule); - typedef bool (*InfoFP)(double, CModInfo&); - InfoFP ZNCModInfo = (InfoFP)dlsym(p, "ZNCModInfo"); - - if (!ZNCModInfo) { + CModuleEntry* (*fpZNCModuleEntry)() = nullptr; + // man dlsym(3) explains this + *reinterpret_cast(&fpZNCModuleEntry) = dlsym(p, "ZNCModuleEntry"); + if (!fpZNCModuleEntry) { + dlclose(p); + sRetMsg = "Could not find ZNCModuleEntry in module [" + sModule + "]"; + return nullptr; + } + const CModuleEntry* pModuleEntry = fpZNCModuleEntry(); + + if (std::strcmp(pModuleEntry->pcVersion, VERSION_STR) || + std::strcmp(pModuleEntry->pcVersionExtra, VERSION_EXTRA)) { + sRetMsg = "Version mismatch for module [" + sModule + + "] (core is " VERSION_STR VERSION_EXTRA + ", module is built for " + + CString(pModuleEntry->pcVersion) + + pModuleEntry->pcVersionExtra + "), recompile this module."; dlclose(p); - sRetMsg = "Could not find ZNCModInfo() in module [" + sModule + "]"; return nullptr; } - if (ZNCModInfo(CModule::GetCoreVersion(), Info)) { - sRetMsg = ""; - bVersionMismatch = false; - } else { - bVersionMismatch = true; - sRetMsg = "Version mismatch, recompile this module."; + if (std::strcmp(pModuleEntry->pcCompileOptions, + ZNC_COMPILE_OPTIONS_STRING)) { + sRetMsg = + "Module [" + sModule + + "] is built incompatibly (core is '" ZNC_COMPILE_OPTIONS_STRING + "', module is '" + + CString(pModuleEntry->pcCompileOptions) + + "'), recompile this module."; + dlclose(p); + return nullptr; } + CTranslationDomainRefHolder translation("znc-" + sModule); + pModuleEntry->fpFillModInfo(Info); + + sRetMsg = ""; return p; } diff --git a/src/znc.cpp b/src/znc.cpp index ab103fff..ba86fb76 100644 --- a/src/znc.cpp +++ b/src/znc.cpp @@ -139,43 +139,15 @@ CString CZNC::GetTag(bool bIncludeVersion, bool bHTML) { } CString CZNC::GetCompileOptionsString() { - return "IPv6: " -#ifdef HAVE_IPV6 - "yes" -#else - "no" -#endif - ", SSL: " -#ifdef HAVE_LIBSSL - "yes" -#else - "no" -#endif - ", DNS: " -#ifdef HAVE_THREADED_DNS - "threads" -#else - "blocking" -#endif - ", charset: " -#ifdef HAVE_ICU - "yes" -#else - "no" -#endif - ", build: " + // Build system doesn't affect ABI + return ZNC_COMPILE_OPTIONS_STRING + CString( + ", build: " #ifdef BUILD_WITH_CMAKE - "cmake" + "cmake" #else - "autoconf" + "autoconf" #endif - ", i18n: " -#ifdef HAVE_I18N - "yes" -#else - "no" -#endif - ; + ); } CString CZNC::GetUptime() const {