Re: [PATCH spice-streaming-agent 1/2] Implement version checking for plugins without violating ODR

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 22, 2017 at 04:33:23PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> 
> 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.
> 
> GCC respects the Standard C++ ABI on most platforms, so I believe this
> could be made to work despite the violation as long as we never
> changed the order of declarations, etc. But the point of the ABI check
> is that we should be able to change the agent interface in any way we
> want and be safe. And technically, the safe cases would still be an
> ODR violation that relies on knowledge of implementation details.
> 
> Another issue is that we don't want to have to rely on the plugins to
> do the version checks. It is more robust to do it in the agent, where
> it ensures that it is done in a consistent way and with consistent
> error messages. As a matter of fact, the new check is robust against
> older plugins and will not load them.
> 
> The mechanism implemented here is similar to what libtool uses.
> It lets any interface update be marked as either compatible with
> earlier plugins or incompatible.

I don't think fixing that issue with Agent::PluginVersionIsCompatible
requires switching to that libtool ABI versioning, does it? In other
word, do these 2 changes (a bug fix, and a functional change) have to go
in the same commit?

Christophe

> 
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
>  include/spice-streaming-agent/plugin.hpp | 48 +++++++++++++++++---------------
>  src/concrete-agent.cpp                   | 35 ++++++++++++-----------
>  src/concrete-agent.hpp                   |  4 ---
>  src/mjpeg-fallback.cpp                   |  3 --
>  4 files changed, 43 insertions(+), 47 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
> index 727cb3b..1a58856 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -22,11 +22,20 @@ namespace SpiceStreamingAgent {
>  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, then increment PluginInterfaceAge.
> + * [INCOMPATIBLE CHANGE] If any interfaces have been removed or changed
> + * since the last public release, then set PluginInterfaceAge to 0.
>   */
> -enum Constants : unsigned { PluginVersion = 0x100u };
> +enum Constants : unsigned {
> +    PluginInterfaceVersion = 0,
> +    PluginInterfaceAge = 0
> +};
>  
>  enum Ranks : unsigned {
>      /// this plugin should not be used
> @@ -102,20 +111,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;
> @@ -135,18 +130,25 @@ typedef bool PluginInitFunc(SpiceStreamingAgent::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" SpiceStreamingAgent::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 192054a..be0ec66 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -16,28 +16,11 @@
>  using namespace std;
>  using namespace SpiceStreamingAgent;
>  
> -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(shared_ptr<Plugin>(&plugin));
> @@ -85,6 +68,24 @@ void ConcreteAgent::LoadPlugin(const char *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);
> +        return;
> +    }
> +    if (*version < PluginInterfaceVersion - PluginInterfaceAge ||
> +        *version > PluginInterfaceVersion) {
> +        syslog(LOG_ERR,
> +               "error loading plugin %s: plugin interface version %u, "
> +               "agent accepts version %u...%u",
> +               plugin_filename, *version,
> +               PluginInterfaceVersion - PluginInterfaceAge,
> +               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 828368b..bcf0a76 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -25,16 +25,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 char *directory);
>      // pointer must remain valid
>      void AddOption(const char *name, const char *value);
>      FrameCapture *GetBestFrameCapture();
> -    bool PluginVersionIsCompatible(unsigned pluginVersion) const override;
>  private:
>      void LoadPlugin(const char *plugin_filename);
>      std::vector<std::shared_ptr<Plugin>> plugins;
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index f41e68a..27505e6 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -208,9 +208,6 @@ void MjpegPlugin::ParseOptions(const ConfigureOption *options)
>  static bool
>  mjpeg_plugin_init(Agent* agent)
>  {
> -    if (agent->Version() != PluginVersion)
> -        return false;
> -
>      std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
>  
>      plugin->ParseOptions(agent->Options());
> -- 
> 2.13.5 (Apple Git-94)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]