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@xxxxxxxxxx> wrote: > > From: Christophe de Dinechin <dinechin@xxxxxxxxxx> > > 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@xxxxxxxxxx> > --- > 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 > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel