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 Fri, 2018-02-16 at 07:23 -0500, Frediano Ziglio wrote:
> > > 
> > > > 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.
> 
> 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
> 

Acked and merged

Frediano
_______________________________________________
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]