[Spice-devel] [PATCH spice-streaming-agent 2/3] Change numbering schema

[Christophe de Dinechin]

There was also some input from Lukas, which I had integrated here: https://github.com/c3d/spice-streaming-agent/commit/6f04ffd8d03c9b43533cd7082cb755bb79accf90. Basically, using an enum class.

And there were some comments from either you or Christophe, I forgot, which I tried to capture here: https://github.com/c3d/spice-streaming-agent/commit/65ce0c9d747baaa81a4789d7f7c18f6a4f49732c. Mostly clarifying binary compatibility guidelines.

With or without the changes above, OK with me.


Christophe


> On 19 Apr 2018, at 18:48, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> A major.minor numbering scheme is not ideal for ABI checks.
> In particular, it makes it difficult to react to an incompatibility
> that was detected post release.
> 
> [More info]
> 
> The major.minor numbering scheme initially selected makes it harder
> to fixes cases where an incompatibility was detected after release.
> 
> For example, the major.minor version checking assumes that agent 1.21
> is compatible with plugins 1.21, 1.13 or 1.03. If later testing
> shows that 1.13 actually introduced an incompatiliy, you have to
> special-case 1.13 in the compatibiliy check.
> 
> An approach that does not have this problem is to rely on incremented
> version numbers, with a "current" and "oldest compatible" version
> number. This is used for example by libtools [1].
> 
> Since the change required for #1 and #2  introduces an ABI break,
> it is as good a time as any to also change the numbering scheme,
> since changing it later would introduce another unnecessary ABI break.
> 
> [1] https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
> include/spice-streaming-agent/plugin.hpp | 21 ++++++++++++++++-----
> src/concrete-agent.cpp                   | 28 +++++++---------------------
> src/concrete-agent.hpp                   |  1 -
> 3 files changed, 23 insertions(+), 27 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
> index 6784ebc..f95fb11 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -23,11 +23,22 @@ namespace streaming_agent {
> 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,
> + * leave PluginInterfaceOldestCompatibleVersion identical.
> + * [INCOMPATIBLE CHANGE] If any interfaces have been removed or changed since the last release,
> + * set PluginInterfaceOldestCompatibleVersion to PluginInterfaceVersion.
> + * [DETECTED INCOMPATIBILITY]: If an incompatibility is detected after a release,
> + * set PluginInterfaceOldestCompatibleVersion to the last known compatible version.
>  */
> -enum Constants : unsigned { PluginVersion = 0x100u };
> +enum Constants : unsigned {
> +    PluginInterfaceVersion = 1,
> +    PluginInterfaceOldestCompatibleVersion = 1
> +};
> 
> enum Ranks : unsigned {
>     /// this plugin should not be used
> @@ -123,7 +134,7 @@ typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
> #ifndef SPICE_STREAMING_AGENT_PROGRAM
> /*!
>  * Plugin interface version
> - * Each plugin should define this variable and set it to PluginVersion
> + * 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;
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 58ce9c4..eb4f333 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -15,28 +15,11 @@
> 
> using namespace spice::streaming_agent;
> 
> -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 = PluginVersion;
> -    return MajorVersion(version) == MajorVersion(pluginVersion) &&
> -        MinorVersion(version) >= MinorVersion(pluginVersion);
> -}
> -
> void ConcreteAgent::Register(Plugin& plugin)
> {
>     plugins.push_back(std::shared_ptr<Plugin>(&plugin));
> @@ -90,11 +73,14 @@ void ConcreteAgent::LoadPlugin(const std::string &plugin_filename)
>                plugin_filename.c_str());
>         return;
>     }
> -    if (!PluginVersionIsCompatible(*version)) {
> +    if (*version < PluginInterfaceOldestCompatibleVersion ||
> +        *version > PluginInterfaceVersion) {
>         syslog(LOG_ERR,
> -               "error loading plugin %s: plugin interface version %u.%u not accepted",
> -               plugin_filename.c_str(),
> -               MajorVersion(*version), MinorVersion(*version));
> +               "error loading plugin %s: plugin interface version %u, "
> +               "agent accepts version %u...%u",
> +               plugin_filename.c_str(), *version,
> +               PluginInterfaceOldestCompatibleVersion,
> +               PluginInterfaceVersion);
>         return;
>     }
> 
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index c0cf9ba..c631916 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -34,7 +34,6 @@ public:
>     void AddOption(const char *name, const char *value);
>     FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>& codecs);
> private:
> -    bool PluginVersionIsCompatible(unsigned pluginVersion) const;
>     void LoadPlugin(const std::string &plugin_filename);
>     std::vector<std::shared_ptr<Plugin>> plugins;
>     std::vector<ConcreteConfigureOption> options;
> -- 
> 2.14.3
>