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 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




[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]