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

[Frediano Ziglio]

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

Yes, more clean indeed.

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

Suggestions? It's not a problem and I don't think it's common, depends
on the style but I don't think is a common style like 90% does this,
obviously is you say "it's standard in MFC classes" this could be true
but we are not MFC.

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

This would make code less readable or risk to have it not "ended" like this.

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

Would require something like this which was ugly at the beginning.
Or to make sure to terminate at the end.

> > -}
> > -
> >   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());
> > 
> 
>