[Spice-devel] [PATCH spice-streaming-agent v2 2/2] explicit instead of static registration for built-in plugins
Frediano Ziglio
fziglio at redhat.com
Tue Feb 6 09:51:29 UTC 2018
>
> > On 5 Feb 2018, at 15:29, Lukáš Hrázký <lhrazky at 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ý <lhrazky at 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 <dinechin at 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.
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
More information about the Spice-devel
mailing list