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

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

 



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 ?

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 ?

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

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




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