>
> > On 6 Feb 2018, at 10:51, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >
> >>>
> >>> 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 ;-)
> >>
> >
> > 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++.
> >
Was forgetting about this.
I think there's more agreement on removing this feature.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel