[Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR
Christophe de Dinechin
christophe at dinechin.org
Wed Nov 22 15:33:23 UTC 2017
From: Christophe de Dinechin <dinechin at redhat.com>
The C++ One Definition Rule (ODR) states that all translation units
must see the same definitions. In the current code, when we call
Agent::PluginVersionIsCompatible from the plugin, it is an ODR
violation as soon as we have made any change in the Agent class
compared to what the plugin was compiled against.
GCC respects the Standard C++ ABI on most platforms, so I believe this
could be made to work despite the violation as long as we never
changed the order of declarations, etc. But the point of the ABI check
is that we should be able to change the agent interface in any way we
want and be safe. And technically, the safe cases would still be an
ODR violation that relies on knowledge of implementation details.
Another issue is that we don't want to have to rely on the plugins to
do the version checks. It is more robust to do it in the agent, where
it ensures that it is done in a consistent way and with consistent
error messages. As a matter of fact, the new check is robust against
older plugins and will not load them.
The mechanism implemented here is similar to what libtool uses.
It lets any interface update be marked as either compatible with
earlier plugins or incompatible.
Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
---
include/spice-streaming-agent/plugin.hpp | 48 +++++++++++++++++---------------
src/concrete-agent.cpp | 35 ++++++++++++-----------
src/concrete-agent.hpp | 4 ---
src/mjpeg-fallback.cpp | 3 --
4 files changed, 43 insertions(+), 47 deletions(-)
diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
index 727cb3b..1a58856 100644
--- a/include/spice-streaming-agent/plugin.hpp
+++ b/include/spice-streaming-agent/plugin.hpp
@@ -22,11 +22,20 @@ namespace SpiceStreamingAgent {
class FrameCapture;
/*!
- * Plugin version, only using few bits, schema is 0xMMmm
- * where MM is major and mm is the minor, can be easily expanded
- * using more bits in the future
+ * Plugins use a versioning system similar to that implemented by libtool
+ * http://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
+ * Update the version information as follows:
+ * [ANY CHANGE] If any interfaces have been added, removed, or changed
+ * since the last update, increment PluginInterfaceVersion.
+ * [COMPATIBLE CHANGE] If interfaces have only been added since the last
+ * public release, then increment PluginInterfaceAge.
+ * [INCOMPATIBLE CHANGE] If any interfaces have been removed or changed
+ * since the last public release, then set PluginInterfaceAge to 0.
*/
-enum Constants : unsigned { PluginVersion = 0x100u };
+enum Constants : unsigned {
+ PluginInterfaceVersion = 0,
+ PluginInterfaceAge = 0
+};
enum Ranks : unsigned {
/// this plugin should not be used
@@ -102,20 +111,6 @@ class Agent
{
public:
/*!
- * Get agent version.
- * Plugin should check the version for compatibility before doing
- * everything.
- * \return version specified like PluginVersion
- */
- virtual unsigned Version() const=0;
-
- /*!
- * Check if a given plugin version is compatible with this agent
- * \return true is compatible
- */
- virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const=0;
-
- /*!
* Register a plugin in the system.
*/
virtual void Register(Plugin& plugin)=0;
@@ -135,18 +130,25 @@ typedef bool PluginInitFunc(SpiceStreamingAgent::Agent* agent);
#ifndef SPICE_STREAMING_AGENT_PROGRAM
/*!
+ * Plugin interface version
+ * Each plugin should define this variable and set it to PluginInterfaceVersion
+ * That version will be checked by the agent before executing any plugin code
+ */
+extern "C" unsigned spice_streaming_agent_plugin_interface_version;
+
+/*!
* Plugin main entry point.
- * Plugins should check if the version of the agent is compatible.
- * If is compatible should register itself to the agent and return
- * true.
- * If is not compatible can decide to stay in memory or not returning
- * true (do not unload) or false (safe to unload). This is necessary
+ * This entry point is only called if the version check passed.
+ * It should return true if it loaded and initialized successfully.
+ * If the plugin does not initialize and does not want to be unloaded,
+ * it may still return true on failure. This is necessary
* if the plugin uses some library which are not safe to be unloaded.
* This public interface is also designed to avoid exporting data from
* the plugin which could be a problem in some systems.
* \return true if plugin should stay loaded, false otherwise
*/
extern "C" SpiceStreamingAgent::PluginInitFunc spice_streaming_agent_plugin_init;
+
#endif
#endif // SPICE_STREAMING_AGENT_PLUGIN_HPP
diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 192054a..be0ec66 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -16,28 +16,11 @@
using namespace std;
using namespace SpiceStreamingAgent;
-static inline unsigned MajorVersion(unsigned version)
-{
- return version >> 8;
-}
-
-static inline unsigned MinorVersion(unsigned version)
-{
- return version & 0xffu;
-}
-
ConcreteAgent::ConcreteAgent()
{
options.push_back(ConcreteConfigureOption(nullptr, nullptr));
}
-bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
-{
- unsigned version = Version();
- return MajorVersion(version) == MajorVersion(pluginVersion) &&
- MinorVersion(version) >= MinorVersion(pluginVersion);
-}
-
void ConcreteAgent::Register(Plugin& plugin)
{
plugins.push_back(shared_ptr<Plugin>(&plugin));
@@ -85,6 +68,24 @@ void ConcreteAgent::LoadPlugin(const char *plugin_filename)
return;
}
+ unsigned *version =
+ (unsigned *) dlsym(dl, "spice_streaming_agent_plugin_interface_version");
+ if (!version) {
+ syslog(LOG_ERR, "error loading plugin %s: no version information",
+ plugin_filename);
+ return;
+ }
+ if (*version < PluginInterfaceVersion - PluginInterfaceAge ||
+ *version > PluginInterfaceVersion) {
+ syslog(LOG_ERR,
+ "error loading plugin %s: plugin interface version %u, "
+ "agent accepts version %u...%u",
+ plugin_filename, *version,
+ PluginInterfaceVersion - PluginInterfaceAge,
+ PluginInterfaceVersion);
+ return;
+ }
+
try {
PluginInitFunc* init_func =
(PluginInitFunc *) dlsym(dl, "spice_streaming_agent_plugin_init");
diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 828368b..bcf0a76 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -25,16 +25,12 @@ class ConcreteAgent final : public Agent
{
public:
ConcreteAgent();
- unsigned Version() const override {
- return PluginVersion;
- }
void Register(Plugin& plugin) override;
const ConfigureOption* Options() const override;
void LoadPlugins(const char *directory);
// pointer must remain valid
void AddOption(const char *name, const char *value);
FrameCapture *GetBestFrameCapture();
- bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
private:
void LoadPlugin(const char *plugin_filename);
std::vector<std::shared_ptr<Plugin>> plugins;
diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index f41e68a..27505e6 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -208,9 +208,6 @@ void MjpegPlugin::ParseOptions(const ConfigureOption *options)
static bool
mjpeg_plugin_init(Agent* agent)
{
- if (agent->Version() != PluginVersion)
- return false;
-
std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
plugin->ParseOptions(agent->Options());
--
2.13.5 (Apple Git-94)
More information about the Spice-devel
mailing list