[Spice-devel] [PATCH spice-streaming-agent] plugin: Do not use references to pass ownership

[Lukáš Hrázký]

Hi,

On Thu, 2018-09-20 at 14:32 +0100, Frediano Ziglio wrote:
> Usually when references are used ownership is not moved.
> Avoid to use references to confuse code reader.
> Pointers and references have same ABI so the change does not
> break plugin ABI.

I don't think passing raw pointers over API is a clean solution in
C++11. Especially since we actually have a unique_ptr in the plugins'
init functions and there are shared_ptrs in the vector on the agent
side. It's also unsafe in case of bad_alloc while creating the
shared_ptr.

We do not guarantee ABI stability at this stage anyway, so why not put
at least the shared_ptr on the API. unique_ptr would be better if the
Plugins can be made movable, which probably makes sense, since you do
want to transfer the ownership and not have multiple shared_ptrs
pointing to the Plugin.

Cheers,
Lukas

> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  include/spice-streaming-agent/plugin.hpp | 3 ++-
>  src/concrete-agent.cpp                   | 4 ++--
>  src/concrete-agent.hpp                   | 2 +-
>  src/gst-plugin.cpp                       | 2 +-
>  src/mjpeg-fallback.cpp                   | 2 +-
>  5 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
> index e9acf2d..5845272 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -104,8 +104,9 @@ class Agent
>  public:
>      /*!
>       * Register a plugin in the system.
> +     * Agent will take ownership of the plugin.
>       */
> -    virtual void Register(Plugin& plugin) = 0;
> +    virtual void Register(Plugin* plugin) = 0;
>  
>      /*!
>       * Get options array.
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 1f8b4b4..7abc38f 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -37,9 +37,9 @@ bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
>          MinorVersion(version) >= MinorVersion(pluginVersion);
>  }
>  
> -void ConcreteAgent::Register(Plugin& plugin)
> +void ConcreteAgent::Register(Plugin* plugin)
>  {
> -    plugins.push_back(std::shared_ptr<Plugin>(&plugin));
> +    plugins.push_back(std::shared_ptr<Plugin>(plugin));
>  }
>  
>  const ConfigureOption* ConcreteAgent::Options() const
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index c0cf9ba..fd08cd5 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -27,7 +27,7 @@ class ConcreteAgent final : public Agent
>  {
>  public:
>      ConcreteAgent();
> -    void Register(Plugin& plugin) override;
> +    void Register(Plugin* plugin) override;
>      const ConfigureOption* Options() const override;
>      void LoadPlugins(const std::string &directory);
>      // pointer must remain valid
> diff --git a/src/gst-plugin.cpp b/src/gst-plugin.cpp
> index 6eb9e7e..4060942 100644
> --- a/src/gst-plugin.cpp
> +++ b/src/gst-plugin.cpp
> @@ -456,7 +456,7 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
>  
>      plugin->ParseOptions(agent->Options());
>  
> -    agent->Register(*plugin.release());
> +    agent->Register(plugin.release());
>  
>      return true;
>  }
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 08353f2..dd72558 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -188,7 +188,7 @@ bool MjpegPlugin::Register(Agent* agent)
>          syslog(LOG_ERR, "Error parsing plugin option: %s", e.what());
>      }
>  
> -    agent->Register(*plugin.release());
> +    agent->Register(plugin.release());
>  
>      return true;
>  }