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

[Jonathon Jongsma]

Looks reasonable to me.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

On Thu, 2018-09-27 at 14:10 +0100, Frediano Ziglio wrote:
> Usually when references are used ownership is not moved.
> Avoid to use references to confuse code reader.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  include/spice-streaming-agent/plugin.hpp | 4 +++-
>  src/concrete-agent.cpp                   | 4 ++--
>  src/concrete-agent.hpp                   | 2 +-
>  src/gst-plugin.cpp                       | 4 ++--
>  src/mjpeg-fallback.cpp                   | 4 ++--
>  5 files changed, 10 insertions(+), 8 deletions(-)
> 
> Changes since v1:
> - use std::shared_ptr
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp
> b/include/spice-streaming-agent/plugin.hpp
> index e9acf2d..3b265d9 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -8,6 +8,7 @@
>  #define SPICE_STREAMING_AGENT_PLUGIN_HPP
>  
>  #include <spice/enums.h>
> +#include <memory>
>  
>  /*!
>   * \file
> @@ -104,8 +105,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(const std::shared_ptr<Plugin>& plugin) =
> 0;
>  
>      /*!
>       * Get options array.
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 1f8b4b4..f94aead 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(const std::shared_ptr<Plugin>& plugin)
>  {
> -    plugins.push_back(std::shared_ptr<Plugin>(&plugin));
> +    plugins.push_back(plugin);
>  }
>  
>  const ConfigureOption* ConcreteAgent::Options() const
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index c0cf9ba..99dcf54 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(const std::shared_ptr<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..097ef9d 100644
> --- a/src/gst-plugin.cpp
> +++ b/src/gst-plugin.cpp
> @@ -452,11 +452,11 @@ SPICE_STREAMING_AGENT_PLUGIN(agent)
>  {
>      gst_init(nullptr, nullptr);
>  
> -    std::unique_ptr<GstreamerPlugin> plugin(new GstreamerPlugin());
> +    auto plugin = std::make_shared<GstreamerPlugin>();
>  
>      plugin->ParseOptions(agent->Options());
>  
> -    agent->Register(*plugin.release());
> +    agent->Register(plugin);
>  
>      return true;
>  }
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 08353f2..8081007 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -180,7 +180,7 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType()
> const {
>  
>  bool MjpegPlugin::Register(Agent* agent)
>  {
> -    std::unique_ptr<MjpegPlugin> plugin(new MjpegPlugin());
> +    auto plugin = std::make_shared<MjpegPlugin>();
>  
>      try {
>          plugin->ParseOptions(agent->Options());
> @@ -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);
>  
>      return true;
>  }