[spice-streaming-agent,2/3] Change numbering schema

Submitted by Frediano Ziglio on April 18, 2018, 11:47 a.m.

Details

Message ID 20180418114743.26072-3-fziglio@redhat.com
State New
Headers show
Series "Split plugin version check patch" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio April 18, 2018, 11:47 a.m.
From: Christophe de Dinechin <dinechin@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@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(-)

Patch hide | download patch | download mbox

diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
index 90651dd..0ec5040 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;

Comments

On Wed, Apr 18, 2018 at 12:47:42PM +0100, Frediano Ziglio wrote:
> From: Christophe de Dinechin <dinechin@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.

Fwiw, I'm not sure yet I fully understand the rationale behind this, and
the various pros and cons
(see https://lists.freedesktop.org/archives/spice-devel/2018-April/042956.html
and https://lists.freedesktop.org/archives/spice-devel/2018-April/043175.html)
The rationale mentioned in
https://lists.freedesktop.org/archives/spice-devel/2018-April/043151.html
would probably be good to have here if we decide to go with this
versioning.

Christophe