[spice-streaming-agent,1/2] Implement version checking for plugins without violating ODR

Submitted by Christophe de Dinechin on Nov. 22, 2017, 3:33 p.m.

Details

Message ID 20171122153324.63021-2-christophe@dinechin.org
State New
Headers show
Series "Let the agent check plugin versions to avoid ODR violations" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Nov. 22, 2017, 3:33 p.m.
From: Christophe de Dinechin <dinechin@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@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(-)

Patch hide | download patch | download mbox

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());

Comments

> 
> From: Christophe de Dinechin <dinechin@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.
> 

This currently is not true in many ABI unless you add a vfunc before
Agent::PluginVersionIsCompatible which you are not supposed to do.

> 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.
> 

Any ABI came with implementation details, if the compiler decide
to change the registers used there's no way to maintain an ABI.

> 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.
> 

No, to read that variable you need to load the plugin, what is changing
is that you don't need to call a function. Personally having an ABI
relying in exporting variables is not that safe and portable.

> 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.
> 

How are you going to implement that a plugin support from version X
to version Y?

> Signed-off-by: Christophe de Dinechin <dinechin@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;
> +

Is not a good idea to export variables. In some environment variable
are copied into the executable. But I can do some test about it, I
think using dynamic linking can be safe.
Also you point to libtool versioning but how to get the age with
this interface?
Or the API version itself define the range?

> +/*!
>   * 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());

Was wondering about compatible updates.
Supposing we start at version 1.
1) we add API to Plugin. Version will be 2, agent knows the version
   of plugin is 2 and that can safely call the new API. If we load
   an old plugin this has version 1 and we don't call the new API.
   This works;
2) we add API to Agent. Version will be 2. Plugin versions 2 knows
   this new API and will use. Plugin versions 1 don't know and
   won't use. Works too.

Internally we discussed and we agreed that current version is
still development, not having a public release.

So no objections about this patch, beside exporting a variable.
I think is safe at the end, I'll have a quick check just to make sure.

Frediano
> 
> From: Christophe de Dinechin <dinechin@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@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;
> +    }
> +

Don't you want to unload the plugin in case of version errors?

>      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());

Did tests for the variable case. Is fine, works correctly.

Beside the unload the patch is good for me.

Frediano
> On 22 Nov 2017, at 18:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin@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@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;
>> +    }
>> +
> 
> Don't you want to unload the plugin in case of version errors?

Well, you explained in your comment for the plugin init why it was unsafe.
Can we document that it is not allowed and force unload, i.e. if
the plugin wants to load a lib it can’t unload, it has to do it during
init, not e,g, through constructors?

> 
>>     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());
> 
> Did tests for the variable case. Is fine, works correctly.
> 
> Beside the unload the patch is good for me.
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Nov 22, 2017 at 04:33:23PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@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.

I don't think fixing that issue with Agent::PluginVersionIsCompatible
requires switching to that libtool ABI versioning, does it? In other
word, do these 2 changes (a bug fix, and a functional change) have to go
in the same commit?

Christophe

> 
> Signed-off-by: Christophe de Dinechin <dinechin@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)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Nov 22, 2017 at 06:19:34PM +0100, Christophe Fergeau wrote:
> On Wed, Nov 22, 2017 at 04:33:23PM +0100, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin <dinechin@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.
> 
> I don't think fixing that issue with Agent::PluginVersionIsCompatible
> requires switching to that libtool ABI versioning, does it? In other
> word, do these 2 changes (a bug fix, and a functional change) have to go
> in the same commit?

Oh, and just realized that the log does not have any explanation of how
you fixed this bug, nor how the suggested versioning works.

Christophe
> 
> > On 22 Nov 2017, at 18:04, Frediano Ziglio <fziglio@redhat.com> wrote:
> > 
> >> 
> >> From: Christophe de Dinechin <dinechin@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@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;
> >> +    }
> >> +
> > 
> > Don't you want to unload the plugin in case of version errors?
> 
> Well, you explained in your comment for the plugin init why it was unsafe.
> Can we document that it is not allowed and force unload, i.e. if
> the plugin wants to load a lib it can’t unload, it has to do it during
> init, not e,g, through constructors?
> 

I was wondering what other software do. I checked both Xorg and GStreamer
sources. They both unload the driver/plugin.

> > 
> >>     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());
> > 
> > Did tests for the variable case. Is fine, works correctly.
> > 
> > Beside the unload the patch is good for me.
> > 

Frediano