Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Frediano Ziglio]

> 
> > On 22 Nov 2017, at 18:04, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> 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 
> >> ---
> >> 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 

Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Christophe Fergeau]

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


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Christophe Fergeau]

On Wed, Nov 22, 2017 at 04:33:23PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> 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 
> ---
>  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. 

Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Christophe de Dinechin]

> On 22 Nov 2017, at 18:04, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> 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 
>> ---
>> 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 

Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> 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 
> ---
>  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" 

Re: [Spice-devel] [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Frediano Ziglio]

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

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

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