[Spice-devel] [PATCH spice-streaming-agent v2 1/2] Build agent object not statically

[Uri Lublin]

On 5/1/19 4:53 PM, Frediano Ziglio wrote:
> Allows to catch possible exception building the object.
> Also will allow to more safely handle logger dependency.

The subject confused me a bit, as I thought about Makefile/Linking.
Perhaps change to something like - make agent object not static

Also see below for some comments.

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Ack.

> ---
>   src/concrete-agent.cpp        | 11 +++--------
>   src/concrete-agent.hpp        |  4 +---
>   src/spice-streaming-agent.cpp | 12 +++++++-----
>   3 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index f94aead..fb1412b 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -25,9 +25,10 @@ static inline unsigned MinorVersion(unsigned version)
>       return version & 0xffu;
>   }
>   
> -ConcreteAgent::ConcreteAgent()
> +ConcreteAgent::ConcreteAgent(const std::vector<ConcreteConfigureOption> &options):
> +    options(options)

Isn't it common to use different names (e.g. opts for the parameter) ?

>   {
> -    options.push_back(ConcreteConfigureOption(nullptr, nullptr));
> +    this->options.push_back(ConcreteConfigureOption(nullptr, nullptr));

Nit - This too can also be done in main when creating options.

Uri.

>   }
>   
>   bool ConcreteAgent::PluginVersionIsCompatible(unsigned pluginVersion) const
> @@ -49,12 +50,6 @@ const ConfigureOption* ConcreteAgent::Options() const
>       return static_cast<const ConfigureOption*>(&options[0]);
>   }
>   
> -void ConcreteAgent::AddOption(const char *name, const char *value)
> -{
> -    // insert before the last {nullptr, nullptr} value
> -    options.insert(--options.end(), ConcreteConfigureOption(name, value));
> -}
> -
>   void ConcreteAgent::LoadPlugins(const std::string &directory)
>   {
>       std::string pattern = directory + "/*.so";
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index 99dcf54..2c2ebc8 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -26,12 +26,10 @@ struct ConcreteConfigureOption: ConfigureOption
>   class ConcreteAgent final : public Agent
>   {
>   public:
> -    ConcreteAgent();
> +    ConcreteAgent(const std::vector<ConcreteConfigureOption> &options);
>       void Register(const std::shared_ptr<Plugin>& plugin) override;
>       const ConfigureOption* Options() const override;
>       void LoadPlugins(const std::string &directory);
> -    // pointer must remain valid
> -    void AddOption(const char *name, const char *value);
>       FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>& codecs);
>   private:
>       bool PluginVersionIsCompatible(unsigned pluginVersion) const;
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 9507a54..039d628 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -41,8 +41,6 @@
>   
>   using namespace spice::streaming_agent;
>   
> -static ConcreteAgent agent;
> -
>   class FormatMessage : public OutboundMessage<StreamMsgFormat, FormatMessage, STREAM_TYPE_FORMAT>
>   {
>   public:
> @@ -231,7 +229,7 @@ static void usage(const char *progname)
>   }
>   
>   static void
> -do_capture(StreamPort &stream_port, FrameLog &frame_log)
> +do_capture(StreamPort &stream_port, FrameLog &frame_log, ConcreteAgent &agent)
>   {
>       unsigned int frame_count = 0;
>       while (!quit_requested) {
> @@ -353,6 +351,8 @@ int main(int argc, char* argv[])
>   
>       setlogmask(LOG_UPTO(LOG_NOTICE));
>   
> +    std::vector<ConcreteConfigureOption> options;
> +
>       while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL)) != -1) {
>           switch (opt) {
>           case 0:
> @@ -371,7 +371,7 @@ int main(int argc, char* argv[])
>                   usage(argv[0]);
>               }
>               *p++ = '\0';
> -            agent.AddOption(optarg, p);
> +            options.push_back(ConcreteConfigureOption(optarg, p));
>               break;
>           }
>           case OPT_LOG_BINARY:
> @@ -401,6 +401,8 @@ int main(int argc, char* argv[])
>       register_interrupts();
>   
>       try {
> +        ConcreteAgent agent(options);
> +
>           // register built-in plugins
>           MjpegPlugin::Register(&agent);
>   
> @@ -418,7 +420,7 @@ int main(int argc, char* argv[])
>           std::thread cursor_updater{CursorUpdater(&stream_port)};
>           cursor_updater.detach();
>   
> -        do_capture(stream_port, frame_log);
> +        do_capture(stream_port, frame_log, agent);
>       }
>       catch (std::exception &err) {
>           syslog(LOG_ERR, "%s", err.what());
>