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

[Frediano Ziglio]

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