> On 6 Feb 2018, at 10:51, Frediano Ziglio <fzig...@redhat.com> wrote:
>
>>>
>>> On 5 Feb 2018, at 15:29, Lukáš Hrázký <lhra...@redhat.com> 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ý <lhra...@redhat.com>
>>> ---
>>> 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 <dinec...@redhat.com>
>>
>> (I’ll leave the ack to Frediano, feel bad about acking while he’s travelling
>> and presumably can’t easily read the patches ;-)
>>
>
> I personally don't find that much simpler and more maintainable.
I believe that the difference of point of view is based on the following
premise:
- You optimized for the case of many static plugins
- Both Lukas and myself believe this will never happen, and that the number of
static plugins should forever remain at 1.
(optimizing for many dynamic plugins is another matter entirely)
Maybe the question should be: why should any plugin be static?
>
> Yes, there's more code but what happens on dependency and code change?
> Adding a new plugin requires to always have a new header, include in the
> main and call the init function. Previously the main don't even know
> that these plugins existed. Is simpler if the plugins are just a few, but
> surely is a way that don't scale. Consider this applied to Linux kernel
> modules and you easily understand the effort (yes, the scale is much higher).
> From the design also currently is the agent class that has the knowledge
> about the plugin list with this patch part of the responsibility is
> given to the main code.
> For me is quite a common pattern usually implemented using some low
> level section management but to avoid going crazy with specific
> compiler/linker options this is the usual way to do in C++.
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
