>
> 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 ?
>
Good idea.
> Some comments below.
>
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > 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 ?
>
Code is moving in that direction. One reason is that doing like
that make sure header are self-dependent.
> > +
> > #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.
>
Done both
> 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());
> > }
> >
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel