Re: [PATCH spice-streaming-agent] Add support log logging statistics from plugins

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On 26 Apr 2019, at 09:13, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> 
>>> 
>>> On 25 Apr 2019, at 11:42, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>> 
>>> ping 2
>>> 
>>>> 
>>>> ping
>>>> 
>>>>> 
>>>>> Allows the plugins to add information to the log.
>>>>> 
>>>>> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
>>>>> ---
>>>>> 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.

What if you had the FrameLog keep a reference or pointer to the agent
and restore the previous logger in its destructor?

(Could be a derived “AgentFrameLog” class if you prefer).

My concern is about someone forgetting the closing agent.SetFrameLog.


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

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]