On Fri, 2018-02-16 at 07:23 -0500, Frediano Ziglio wrote:
> >
> > > 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++.
> > >
>
> Was forgetting about this.
> I think there's more agreement on removing this feature.
Seems so, though we do have only a small sample of voices :) As I said,
feel free to call the ack/nack...
(Or can I consider this email an ack?)
Thanks,
Lukas
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
