Re: [PATCH spice-streaming-agent v2 2/2] explicit instead of static registration for built-in plugins

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 5 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> 
> The static registration (that is, having a static list of pointers to
> static plugin variables and calling a generic function in the main
> initialization code path to register them) allows to add plugins without
> registering each of them explicitly.
> 
> However, in a single codebase, and having very few plugins, there is
> very little advantage to it and the tradeoff for the complexity of this
> initialization is not worth it. A single call for every built-in plugin
> is much more simple and clear.
> 
> Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> ---
> src/Makefile.am               |  2 --
> src/concrete-agent.cpp        |  3 ---
> src/mjpeg-fallback.cpp        |  6 +-----
> src/mjpeg-fallback.hpp        |  1 +
> src/spice-streaming-agent.cpp |  4 ++++
> src/static-plugin.cpp         | 23 -----------------------
> src/static-plugin.hpp         | 35 -----------------------------------
> 7 files changed, 6 insertions(+), 68 deletions(-)
> delete mode 100644 src/static-plugin.cpp
> delete mode 100644 src/static-plugin.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8d5c5bd..590db1f 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -49,8 +49,6 @@ spice_streaming_agent_LDADD = \
> 
> spice_streaming_agent_SOURCES = \
> 	spice-streaming-agent.cpp \
> -	static-plugin.cpp \
> -	static-plugin.hpp \
> 	concrete-agent.cpp \
> 	concrete-agent.hpp \
> 	mjpeg-fallback.cpp \
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 873a69e..c69da43 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -11,7 +11,6 @@
> #include <dlfcn.h>
> 
> #include "concrete-agent.hpp"
> -#include "static-plugin.hpp"
> 
> using namespace std;
> using namespace SpiceStreamingAgent;
> @@ -58,8 +57,6 @@ void ConcreteAgent::AddOption(const char *name, const char *value)
> 
> void ConcreteAgent::LoadPlugins(const string &directory)
> {
> -    StaticPlugin::InitAll(*this);
> -
>     string pattern = directory + "/*.so";
>     glob_t globbuf;
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 7c918a7..d7c676d 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -15,7 +15,6 @@
> #include <syslog.h>
> #include <X11/Xlib.h>
> 
> -#include "static-plugin.hpp"
> #include "jpeg.hpp"
> 
> using namespace std;
> @@ -191,8 +190,7 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType() const {
>     return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> }
> 
> -static bool
> -mjpeg_plugin_init(Agent* agent)
> +bool MjpegPlugin::Register(Agent* agent)
> {
>     if (agent->Version() != PluginVersion)
>         return false;
> @@ -205,5 +203,3 @@ mjpeg_plugin_init(Agent* agent)
> 
>     return true;
> }
> -
> -static StaticPlugin mjpeg_plugin(mjpeg_plugin_init);
> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> index 8044244..0e9ed6a 100644
> --- a/src/mjpeg-fallback.hpp
> +++ b/src/mjpeg-fallback.hpp
> @@ -25,6 +25,7 @@ public:
>     unsigned Rank() override;
>     void ParseOptions(const ConfigureOption *options);
>     SpiceVideoCodecType VideoCodecType() const;
> +    static bool Register(Agent* agent);
> private:
>     MjpegSettings settings = { 10, 80 };
> };
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 94d9d25..8458975 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -34,6 +34,7 @@
> 
> #include "hexdump.h"
> #include "concrete-agent.hpp"
> +#include "mjpeg-fallback.hpp"
> 
> using namespace std;
> using namespace SpiceStreamingAgent;
> @@ -489,6 +490,9 @@ int main(int argc, char* argv[])
>         }
>     }
> 
> +    // register built-in plugins
> +    MjpegPlugin::Register(&agent);
> +
>     agent.LoadPlugins(PLUGINSDIR);
> 
>     register_interrupts();
> diff --git a/src/static-plugin.cpp b/src/static-plugin.cpp
> deleted file mode 100644
> index d5feb22..0000000
> --- a/src/static-plugin.cpp
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -/* Utility to manage registration of plugins compiled statically
> - *
> - * \copyright
> - * Copyright 2017 Red Hat Inc. All rights reserved.
> - */
> -
> -#ifdef HAVE_CONFIG_H
> -#include <config.h>
> -#endif
> -
> -#include <stdlib.h>
> -#include "static-plugin.hpp"
> -
> -using namespace SpiceStreamingAgent;
> -
> -const StaticPlugin *StaticPlugin::list = nullptr;
> -
> -void StaticPlugin::InitAll(Agent& agent)
> -{
> -    for (const StaticPlugin* plugin = list; plugin; plugin = plugin->next) {
> -        plugin->init_func(&agent);
> -    }
> -}
> diff --git a/src/static-plugin.hpp b/src/static-plugin.hpp
> deleted file mode 100644
> index 5436b41..0000000
> --- a/src/static-plugin.hpp
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -/* Utility to manage registration of plugins compiled statically
> - *
> - * \copyright
> - * Copyright 2017 Red Hat Inc. All rights reserved.
> - */
> -#ifndef SPICE_STREAMING_AGENT_STATIC_PLUGIN_HPP
> -#define SPICE_STREAMING_AGENT_STATIC_PLUGIN_HPP
> -
> -#include <spice-streaming-agent/plugin.hpp>
> -
> -namespace SpiceStreamingAgent {
> -
> -class StaticPlugin final {
> -public:
> -    StaticPlugin(PluginInitFunc init_func):
> -        next(list),
> -        init_func(init_func)
> -    {
> -        list = this;
> -    }
> -    static void InitAll(Agent& agent);
> -private:
> -    // this should be instantiated statically
> -    void *operator new(size_t s);
> -    void *operator new[](size_t s);
> -
> -    const StaticPlugin *const next;
> -    const PluginInitFunc* const init_func;
> -
> -    static const StaticPlugin *list;
> -};
> -
> -}
> -
> -#endif // SPICE_STREAMING_AGENT_STATIC_PLUGIN_HPP

Looks indeed simpler to me.

Reviewed-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>

(I’ll leave the ack to Frediano, feel bad about acking while he’s travelling and presumably can’t easily read the patches ;-)


> -- 
> 2.16.1
> 
> _______________________________________________
> 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]