[Spice-devel] [PATCH spice-streaming-agent v2 2/2] Add support log logging statistics from plugins

[Uri Lublin]

On 5/1/19 4:53 PM, Frediano Ziglio wrote:
> Allows the plugins to add information to the log.

Hi,

Looks good.

Don't you need to bump the version ?

Some comments below.

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>   include/spice-streaming-agent/plugin.hpp |  5 +++++
>   src/concrete-agent.cpp                   | 21 +++++++++++++++++----
>   src/concrete-agent.hpp                   |  8 +++++++-
>   src/frame-log.cpp                        |  9 +++++++++
>   src/frame-log.hpp                        |  2 ++
>   src/spice-streaming-agent.cpp            |  6 +++---
>   6 files changed, 43 insertions(+), 8 deletions(-)
> 
> Change since v1:
> - better ConcreteAgent interface, removed ugly SetFrameLog
> 
> diff --git a/include/spice-streaming-agent/plugin.hpp b/include/spice-streaming-agent/plugin.hpp
> index 3b265d9..a51f060 100644
> --- a/include/spice-streaming-agent/plugin.hpp
> +++ b/include/spice-streaming-agent/plugin.hpp
> @@ -116,6 +116,11 @@ public:
>        * \todo passing options to entry point instead?
>        */
>       virtual const ConfigureOption* Options() const = 0;
> +    /*!
> +     * Write something in the log.
> +     */
> +    __attribute__ ((format (printf, 2, 3)))
> +    virtual void LogStat(const char* format, ...) = 0;
>   };
>   
>   typedef bool PluginInitFunc(spice::streaming_agent::Agent* agent);
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index fb1412b..336bd09 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -5,13 +5,15 @@
>    */
>   
>   #include <config.h>
> +#include "concrete-agent.hpp"
> +#include "frame-log.hpp"

Why move local .h files to the top ?

> +
>   #include <algorithm>
>   #include <syslog.h>
>   #include <glob.h>
>   #include <dlfcn.h>
>   #include <string>
> -
> -#include "concrete-agent.hpp"
> +#include <cstdarg>
>   
>   using namespace spice::streaming_agent;
>   
> @@ -25,8 +27,9 @@ static inline unsigned MinorVersion(unsigned version)
>       return version & 0xffu;
>   }
>   
> -ConcreteAgent::ConcreteAgent(const std::vector<ConcreteConfigureOption> &options):
> -    options(options)
> +ConcreteAgent::ConcreteAgent(const std::vector<ConcreteConfigureOption> &options, FrameLog *logger):
> +    options(options),
> +    logger(logger)
>   {
>       this->options.push_back(ConcreteConfigureOption(nullptr, nullptr));
>   }
> @@ -140,3 +143,13 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const std::set<SpiceVideoCodecT
>       }
>       return nullptr;
>   }
> +
> +void ConcreteAgent::LogStat(const char* format, ...)
> +{
> +    if (logger) {
> +        va_list ap;
> +        va_start(ap, format);
> +        logger->log_statv(format, ap);
> +        va_end(ap);
> +    }
> +}
> diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
> index 2c2ebc8..6d1be35 100644
> --- a/src/concrete-agent.hpp
> +++ b/src/concrete-agent.hpp
> @@ -14,6 +14,8 @@
>   namespace spice {
>   namespace streaming_agent {
>   
> +class FrameLog;
> +
>   struct ConcreteConfigureOption: ConfigureOption
>   {
>       ConcreteConfigureOption(const char *name, const char *value)
> @@ -26,16 +28,20 @@ struct ConcreteConfigureOption: ConfigureOption
>   class ConcreteAgent final : public Agent
>   {
>   public:
> -    ConcreteAgent(const std::vector<ConcreteConfigureOption> &options);
> +    ConcreteAgent(const std::vector<ConcreteConfigureOption> &options,
> +                  FrameLog *logger=nullptr);
>       void Register(const std::shared_ptr<Plugin>& plugin) override;
>       const ConfigureOption* Options() const override;
>       void LoadPlugins(const std::string &directory);
>       FrameCapture *GetBestFrameCapture(const std::set<SpiceVideoCodecType>& codecs);
> +    __attribute__ ((format (printf, 2, 3)))
> +    void LogStat(const char* format, ...) override;
>   private:
>       bool PluginVersionIsCompatible(unsigned pluginVersion) const;
>       void LoadPlugin(const std::string &plugin_filename);
>       std::vector<std::shared_ptr<Plugin>> plugins;
>       std::vector<ConcreteConfigureOption> options;
> +    FrameLog *const logger = nullptr;
>   };
>   
>   }} // namespace spice::streaming_agent
> diff --git a/src/frame-log.cpp b/src/frame-log.cpp
> index 62fffc3..db6b652 100644
> --- a/src/frame-log.cpp
> +++ b/src/frame-log.cpp
> @@ -52,6 +52,15 @@ void FrameLog::log_stat(const char* format, ...)
>       }
>   }
>   
> +void FrameLog::log_statv(const char* format, va_list ap)
> +{
> +    if (log_file) {

Better also check that it's not binary like in log_stat above.

Also as a followup log_stat can call log_statv.

Uri.

> +        fprintf(log_file, "%" PRIu64 ": ", get_time());
> +        vfprintf(log_file, format, ap);
> +        fputc('\n', log_file);
> +    }
> +}
> +
>   void FrameLog::log_frame(const void* buffer, size_t buffer_size)
>   {
>       if (log_file) {
> diff --git a/src/frame-log.hpp b/src/frame-log.hpp
> index 8503345..a104723 100644
> --- a/src/frame-log.hpp
> +++ b/src/frame-log.hpp
> @@ -24,6 +24,8 @@ public:
>   
>       __attribute__ ((format (printf, 2, 3)))
>       void log_stat(const char* format, ...);
> +    __attribute__ ((format (printf, 2, 0)))
> +    void log_statv(const char* format, va_list ap);
>       void log_frame(const void* buffer, size_t buffer_size);
>   
>       static uint64_t get_time();
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 039d628..49f5dc4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -401,15 +401,15 @@ int main(int argc, char* argv[])
>       register_interrupts();
>   
>       try {
> -        ConcreteAgent agent(options);
> +        FrameLog frame_log(log_filename, log_binary, log_frames);
> +
> +        ConcreteAgent agent(options, &frame_log);
>   
>           // register built-in plugins
>           MjpegPlugin::Register(&agent);
>   
>           agent.LoadPlugins(pluginsdir);
>   
> -        FrameLog frame_log(log_filename, log_binary, log_frames);
> -
>           for (const std::string& arg: old_args) {
>               frame_log.log_stat("Args: %s", arg.c_str());
>           }
>