On Mon, 2018-03-26 at 16:57 +0200, Christophe de Dinechin wrote: > > On 23 Mar 2018, at 14:53, Lukáš Hrázký <lhra...@redhat.com> wrote: > > > > On Fri, 2018-03-23 at 13:05 +0100, Christophe de Dinechin wrote: > > > From: Christophe de Dinechin <dinec...@redhat.com> > > > > > > This change addresses three issues related to plugin version checking: > > > > > > 1. It is possible for plugins to bypass version checking or do it wrong > > > (as a matter of fact, the mjpeg fallback sets a bad example) > > > > > > 2. The current plugin version check violates the C++ ODR, i.e. > > > it relies on undefined behaviors when the header used to compile > > > the plugin and to compile the agent are not identical, > > > > > > 3. 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] > > > > > > 1. Make it impossible to bypass version checking > > > > > > The current code depends on the plugin implementing the version check > > > correctly by calling PluginVersionIsCompatible. To make things worse, > > > the only publicly available example gets this wrong and uses an > > > ad-hoc version check, so anybody copy-pasting this code will get it > > > wrong. > > > > > > It is more robust to do the version check in the agent before calling > > > any method in the plugin. It ensures that version checking cannot be > > > bypassed, is done consistently and generates consistent error messages. > > > > > > As an indication that the aproach is robust, the new check correctly > > > refuses to load older plugins that use the old version checking method: > > > > > > spice-streaming-agent[5167]: > > > error loading plugin [...].so: no version information > > > > > > 2. ODR-related problems > > > > > > The C++ One Definition Rule (ODR) states that all translation units > > > must see the same definitions. In the current code, when we call > > > Agent::PluginVersionIsCompatible from the plugin, it is an ODR > > > violation as soon as we have made any change in the Agent class > > > compared to what the plugin was compiled against. > > > > > > The current code therefore relies on implementation dependent knowlege > > > of how virtual functions are laid out in the vtable, and puts > > > unnecessary constraints on the changes allowed in the classes > > > (e.g. it's not allowed to put anything before some member functions) > > > > > > 3. Major.minor numbering scheme > > > > > > 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. > > > > Great! AFAIK we haven't made a release yet so we don't need to worry > > about ABI breakage yet? > > Well, I’d recommend we take the habit of incrementing if we make significant > changes, as a way to warn fellow SPICE developers that something changed.
Ok, good idea. > > > > > [1] > > > https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html > > > > > > Signed-off-by: Christophe de Dinechin <dinec...@redhat.com> > > > --- > > > include/spice-streaming-agent/plugin.hpp | 50 > > > +++++++++++++++++--------------- > > > src/concrete-agent.cpp | 35 +++++++++++----------- > > > src/concrete-agent.hpp | 4 --- > > > src/mjpeg-fallback.cpp | 3 -- > > > 4 files changed, 45 insertions(+), 47 deletions(-) > > > > > > diff --git a/include/spice-streaming-agent/plugin.hpp > > > b/include/spice-streaming-agent/plugin.hpp > > > index e08e3a6..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 > > > +}; > > > > This is still not too pretty, consider at least renaming Constants to > > something better, or even use something like: > > > > struct PluginInterfaceVersion { > > static constexpr uint16_t current = 1; > > static constexpr uint16_t oldest_compatible = 1; > > }; > > > > For the encapsulation? > > The two values are of the same type, and can be compared only with one > another, which is what the enum type says. This is completely lost with your > suggestion. Well, they are also integer numbers... > That being said, Constants is not a very good name choice, it makes it sound > like it’s just a bag of random unrelated constants. And it would be > beneficial to use an enum class here to enforce the “compare with one > another". > > What about the additional patch below? > > Author: Christophe de Dinechin <dinec...@redhat.com> > Date: Mon Mar 26 16:26:57 2018 +0200 > > Use an enumeration class for the plugin interface version > > diff --git a/include/spice-streaming-agent/plugin.hpp > b/include/spice-streaming-agent/plugin.hpp > index b01cd82..a98ee58 100644 > --- a/include/spice-streaming-agent/plugin.hpp > +++ b/include/spice-streaming-agent/plugin.hpp > @@ -35,9 +35,9 @@ class FrameCapture; > * [DETECTED INCOMPATIBILITY]: If an incompatibility is detected after a > release, > * set PluginInterfaceOldestCompatibleVersion to the last known compatible > version. > */ > -enum Constants : unsigned { > - PluginInterfaceVersion = 1, > - PluginInterfaceOldestCompatibleVersion = 1 > +enum class PluginInterfaceVersion : unsigned { > + Current = 1, > + OldestCompatible = 1 > }; > > enum Ranks : unsigned { > @@ -154,8 +154,9 @@ extern "C" spice::streaming_agent::PluginInitFunc > spice_streaming_agent_plugin_i > > #define SPICE_STREAMING_AGENT_PLUGIN(agent) \ > __attribute__ ((visibility ("default"))) \ > - unsigned spice_streaming_agent_plugin_interface_version = \ > - spice::streaming_agent::PluginInterfaceVersion; \ > + spice::streaming_agent::PluginInterfaceVersion \ > + spice_streaming_agent_plugin_interface_version = \ > + spice::streaming_agent::PluginInterfaceVersion::Current; \ > \ > __attribute__ ((visibility ("default"))) \ > bool spice_streaming_agent_plugin_init(spice::streaming_agent::Agent* > agent) > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp > index eb4f333..d7b9fbc 100644 > --- a/src/concrete-agent.cpp > +++ b/src/concrete-agent.cpp > @@ -66,21 +66,21 @@ void ConcreteAgent::LoadPlugin(const std::string > &plugin_filename) > return; > } > > - unsigned *version = > - (unsigned *) dlsym(dl, > "spice_streaming_agent_plugin_interface_version"); > + PluginInterfaceVersion *version = > + (PluginInterfaceVersion *) dlsym(dl, > "spice_streaming_agent_plugin_interface_version"); > if (!version) { > syslog(LOG_ERR, "error loading plugin %s: no version information", > plugin_filename.c_str()); > return; > } > - if (*version < PluginInterfaceOldestCompatibleVersion || > - *version > PluginInterfaceVersion) { > + if (*version < PluginInterfaceVersion::OldestCompatible || > + *version > PluginInterfaceVersion::Current) { > syslog(LOG_ERR, > "error loading plugin %s: plugin interface version %u, " > "agent accepts version %u...%u", > plugin_filename.c_str(), *version, > - PluginInterfaceOldestCompatibleVersion, > - PluginInterfaceVersion); > + PluginInterfaceVersion::OldestCompatible, > + PluginInterfaceVersion::Current); > return; > } Great, I haven't realized you can compare enum class items with one another... I wanted to suggest an enum class but kept thinking of numbers and that wouldn't work. Cheers, Lukas _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
