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

[Frediano Ziglio]

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

I still don't agree that a flat schema is better, all cases you presented can
be done with major.minor.

This series was splitting the original huge patch which was complicated to
discuss as including too many changes at a time, that's why we didn't modify
at all the end result making possible to rebase on the new series with a
"git rebase --onto"

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

This is not the schema but the current code check which can be
changed with a range approach; major.minor version schema have
order property like this flat schema you are proposing.

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

An "oldest compatible" version can be defined also with major.minor.
Note that referring to libtool is confusing as libtool refers to binary
compatibility so we are introducing different definition for the same
term making hard to discuss about.

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

As changes #1 and #2 are now separate this comment is now not valid any more.

Frediano

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