> 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?
>
>> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel