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

[Christophe de Dinechin]

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

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;
>>>     }
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel