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