>
> > On 22 Nov 2017, at 18:04, Frediano Ziglio <fziglio@xxxxxxxxxx> 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.
> >>
> >> 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;
> >> + }
> >> +
> >
> > Don't you want to unload the plugin in case of version errors?
>
> Well, you explained in your comment for the plugin init why it was unsafe.
> Can we document that it is not allowed and force unload, i.e. if
> the plugin wants to load a lib it can’t unload, it has to do it during
> init, not e,g, through constructors?
>
I was wondering what other software do. I checked both Xorg and GStreamer
sources. They both unload the driver/plugin.
> >
> >> 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());
> >
> > Did tests for the variable case. Is fine, works correctly.
> >
> > Beside the unload the patch is good for me.
> >
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel