On Fri, 2018-03-23 at 11:50 -0400, Frediano Ziglio wrote: > > > > On Fri, 2018-03-23 at 13:05 +0100, Christophe de Dinechin wrote: > > > From: Christophe de Dinechin <dinechin@xxxxxxxxxx> > > > > > > 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? > > > > > [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 | 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? > > > > Cheers, > > Lukas > > > > I agree with the rename. > Using a structure to define constants seems weird, an enumeration looks > more natural. Well, enumeration looks more natural only because you're used to use it this way, semantically the two parts of the version are in no way enumeration items :) > > > enum Ranks : unsigned { > > > /// this plugin should not be used > > > @@ -103,20 +114,6 @@ class Agent > > > { > > > public: > > > /*! > > > - * Get agent version. > > > - * Plugin should check the version for compatibility before doing > > > - * everything. > > > - * \return version specified like PluginVersion > > > - */ > > > - virtual unsigned Version() const = 0; > > > - > > > - /*! > > > - * Check if a given plugin version is compatible with this agent > > > - * \return true is compatible > > > - */ > > > - virtual bool PluginVersionIsCompatible(unsigned pluginVersion) const = > > > 0; > > > - > > > - /*! > > > * Register a plugin in the system. > > > */ > > > virtual void Register(Plugin& plugin) = 0; > > > @@ -136,18 +133,25 @@ 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 > > > PluginInterfaceVersion > > > + * That version will be checked by the agent before executing any plugin > > > code > > > + */ > > > +extern "C" unsigned spice_streaming_agent_plugin_interface_version; > > > + > > > +/*! > > > * Plugin main entry point. > > > - * Plugins should check if the version of the agent is compatible. > > > - * If is compatible should register itself to the agent and return > > > - * true. > > > - * If is not compatible can decide to stay in memory or not returning > > > - * true (do not unload) or false (safe to unload). This is necessary > > > + * This entry point is only called if the version check passed. > > > + * It should return true if it loaded and initialized successfully. > > > + * If the plugin does not initialize and does not want to be unloaded, > > > + * it may still return true on failure. This is necessary > > > * if the plugin uses some library which are not safe to be unloaded. > > > * This public interface is also designed to avoid exporting data from > > > * the plugin which could be a problem in some systems. > > > * \return true if plugin should stay loaded, false otherwise > > > */ > > > extern "C" spice::streaming_agent::PluginInitFunc > > > spice_streaming_agent_plugin_init; > > > + > > > #endif > > > > > > #endif // SPICE_STREAMING_AGENT_PLUGIN_HPP > > > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp > > > index 4cf70e7..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 = Version(); > > > - return MajorVersion(version) == MajorVersion(pluginVersion) && > > > - MinorVersion(version) >= MinorVersion(pluginVersion); > > > -} > > > - > > > void ConcreteAgent::Register(Plugin& plugin) > > > { > > > plugins.push_back(std::shared_ptr<Plugin>(&plugin)); > > > @@ -83,6 +66,24 @@ void ConcreteAgent::LoadPlugin(const std::string > > > &plugin_filename) > > > return; > > > } > > > > > > + unsigned *version = > > > + (unsigned *) 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) { > > > + syslog(LOG_ERR, > > > + "error loading plugin %s: plugin interface version %u, " > > > + "agent accepts version %u...%u", > > > + plugin_filename.c_str(), *version, > > > + PluginInterfaceOldestCompatibleVersion, > > > + PluginInterfaceVersion); > > > + return; > > > + } > > > + > > > try { > > > PluginInitFunc* init_func = > > > (PluginInitFunc *) dlsym(dl, > > > "spice_streaming_agent_plugin_init"); > > > diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp > > > index 5bca23b..c631916 100644 > > > --- a/src/concrete-agent.hpp > > > +++ b/src/concrete-agent.hpp > > > @@ -27,16 +27,12 @@ class ConcreteAgent final : public Agent > > > { > > > public: > > > ConcreteAgent(); > > > - unsigned Version() const override { > > > - return PluginVersion; > > > - } > > > void Register(Plugin& plugin) override; > > > const ConfigureOption* Options() const override; > > > void LoadPlugins(const std::string &directory); > > > // pointer must remain valid > > > void AddOption(const char *name, const char *value); > > > FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>& > > > codecs); > > > - bool PluginVersionIsCompatible(unsigned pluginVersion) const override; > > > private: > > > void LoadPlugin(const std::string &plugin_filename); > > > std::vector<std::shared_ptr<Plugin>> plugins; > > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp > > > index 68c282f..605a4b3 100644 > > > --- a/src/mjpeg-fallback.cpp > > > +++ b/src/mjpeg-fallback.cpp > > > @@ -180,9 +180,6 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType() const > > > { > > > > > > bool MjpegPlugin::Register(Agent* agent) > > > { > > > - if (agent->Version() != PluginVersion) > > > - return false; > > > - > > > std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin()); > > > > > > try { > > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel