[Spice-devel] [PATCH spice-streaming-agent] Add support log logging statistics from plugins
Frediano Ziglio
fziglio at redhat.com
Fri Apr 26 12:13:06 UTC 2019
> > On 26 Apr 2019, at 09:13, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> >>>
> >>> 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.
>
> 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).
>
It seems it add a circular dependency.
I think the issue is that the agent has more lifespan than the log,
maybe this should be changed instead.
No much reason to have "agent" global beside the "AddOption" call which
can be changed instead.
> My concern is about someone forgetting the closing agent.SetFrameLog.
>
Yes, I agree, thanks
>
> >
> >> 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;
> >>>>> }
More information about the Spice-devel
mailing list