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

[Frediano Ziglio]

> 
> > On 25 Apr 2019, at 11:42, Frediano Ziglio <fziglio at redhat.com> wrote:
> > 
> > ping 2
> > 
> >> 
> >> ping
> >> 
> >>> 
> >>> Allows the plugins to add information to the log.
> >>> 
> >>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> >>> ---
> >>> Not entirely happy to export a kind of C function, any suggestion
> >>> welcome
> >>> ---
> >>> include/spice-streaming-agent/plugin.hpp |  5 +++++
> >>> src/concrete-agent.cpp                   | 16 ++++++++++++++--
> >>> src/concrete-agent.hpp                   |  6 ++++++
> >>> src/frame-log.cpp                        |  9 +++++++++
> >>> src/frame-log.hpp                        |  2 ++
> >>> src/spice-streaming-agent.cpp            |  8 ++++++--
> >>> 6 files changed, 42 insertions(+), 4 deletions(-)
> >>> 
> >>> 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 f94aead..d279656 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"
> >>> +
> >>> #include <algorithm>
> >>> #include <syslog.h>
> >>> #include <glob.h>
> >>> #include <dlfcn.h>
> >>> #include <string>
> >>> -
> >>> -#include "concrete-agent.hpp"
> >>> +#include <cstdarg>
> >>> 
> >>> using namespace spice::streaming_agent;
> >>> 
> >>> @@ -145,3 +147,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 99dcf54..aa4d6aa 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)
> >>> @@ -33,11 +35,15 @@ public:
> >>>     // pointer must remain valid
> >>>     void AddOption(const char *name, const char *value);
> >>>     FrameCapture *GetBestFrameCapture(const
> >>>     std::set<SpiceVideoCodecType>&
> >>>     codecs);
> >>> +    void SetFrameLog(FrameLog *logger) { this->logger = logger; }
> >>> +    __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 *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) {
> >>> +        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 9507a54..f262118 100644
> >>> --- a/src/spice-streaming-agent.cpp
> >>> +++ b/src/spice-streaming-agent.cpp
> >>> @@ -401,13 +401,15 @@ int main(int argc, char* argv[])
> >>>     register_interrupts();
> >>> 
> >>>     try {
> >>> +        FrameLog frame_log(log_filename, log_binary, log_frames);
> >>> +
> >>> +        agent.SetFrameLog(&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());
> >>>         }
> >>> @@ -419,8 +421,10 @@ int main(int argc, char* argv[])
> >>>         cursor_updater.detach();
> >>> 
> >>>         do_capture(stream_port, frame_log);
> >>> +        agent.SetFrameLog(nullptr);
> 
> This looks like a bug fix that does not match the commit description.
> Do you want to make it a separate patch? LGTM either way.
> 

SetFrameLog was not present before this patch, nor the field in ConcreteAgent.
I was wondering if i would be better if the logger were instantiated by
ConcreteAgent instead.
These agent.SetFrameLog(nullptr) are more to make sure the pointer is valid.

> Also, should SetFrameLog return previous log pointer to make it
> safer for nested use cases? (follow up)
> 
> 
> >>>     }
> >>>     catch (std::exception &err) {
> >>> +        agent.SetFrameLog(nullptr);
> >>>         syslog(LOG_ERR, "%s", err.what());
> >>>         return EXIT_FAILURE;
> >>>     }

Frediano