[v2,15/24] Create FrameLog class to abstract logging of frames

Submitted by Christophe de Dinechin on Feb. 21, 2018, 5:46 p.m.

Details

Message ID 20180221174636.37758-16-christophe@dinechin.org
State New
Headers show
Series "WIP: Refactor the streaming agent towards a more standard C++ style" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 21, 2018, 5:46 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/spice-streaming-agent.cpp | 99 ++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 39 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 1c7c42d..2c38233 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -48,6 +48,17 @@  namespace spice
 namespace streaming_agent
 {
 
+/* returns current time in micro-seconds */
+static uint64_t get_time(void)
+{
+    struct timeval now;
+
+    gettimeofday(&now, NULL);
+
+    return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
+
+}
+
 class Stream
 {
 public:
@@ -149,6 +160,7 @@  struct FrameMessage : Message<StreamMsgData, FrameMessage>
     }
 };
 
+
 struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
 {
     X11CursorMessage(XFixesCursorImage *cursor)
@@ -198,6 +210,46 @@  private:
 };
 
 
+class FrameLog
+{
+public:
+    FrameLog(const char *filename, bool binary = false);
+    ~FrameLog();
+
+    operator bool() { return log != NULL; }
+    void dump(const void *buffer, size_t length);
+
+private:
+    FILE *log;
+    bool binary;
+};
+
+
+FrameLog::FrameLog(const char *filename, bool binary)
+    : log(filename ? fopen(filename, "wb") : NULL), binary(binary)
+{
+    if (filename && !log) {
+        // We do not abort the program in that case, it's only a warning
+        syslog(LOG_WARNING, "Failed to open hexdump log file '%s': %m\n", filename);
+    }
+}
+
+FrameLog::~FrameLog()
+{
+    if (log)
+        fclose(log);
+}
+
+void FrameLog::dump(const void *buffer, size_t length)
+{
+    if (binary) {
+        fwrite(buffer, length, 1, log);
+    } else {
+        fprintf(log, "%" PRIu64 ": Frame of %zu bytes:\n", get_time(), length);
+        hexdump(buffer, length, log);
+    }
+}
+
 class X11CursorThread
 {
 public:
@@ -266,7 +318,6 @@  void X11CursorThread::cursor_changes()
 
 static bool streaming_requested = false;
 static bool quit_requested = false;
-static bool log_binary = false;
 static std::set<SpiceVideoCodecType> client_codecs;
 
 int Stream::have_something_to_read(int timeout)
@@ -401,17 +452,6 @@  size_t Stream::write_all(const char *what, const void *buf, const size_t len)
     return written;
 }
 
-/* returns current time in micro-seconds */
-static uint64_t get_time(void)
-{
-    struct timeval now;
-
-    gettimeofday(&now, NULL);
-
-    return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
-
-}
-
 static void handle_interrupt(int intr)
 {
     syslog(LOG_INFO, "Got signal %d, exiting", intr);
@@ -434,7 +474,7 @@  static void usage(const char *progname)
     printf("options are:\n");
     printf("\t-p portname  -- virtio-serial port to use\n");
     printf("\t-l file -- log frames to file\n");
-    printf("\t--log-binary -- log binary frames (following -l)\n");
+    printf("\t-b or --log-binary -- log binary frames (following -l)\n");
     printf("\t-d -- enable debug logs\n");
     printf("\t-c variable=value -- change settings\n");
     printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
@@ -445,7 +485,7 @@  static void usage(const char *progname)
 }
 
 static void
-do_capture(Stream &stream, const char *streamport, FILE *f_log)
+do_capture(Stream &stream, const char *streamport, FrameLog &frame_log)
 {
     unsigned int frame_count = 0;
     while (!quit_requested) {
@@ -497,14 +537,8 @@  do_capture(Stream &stream, const char *streamport, FILE *f_log)
                 if (!stream.send<FormatMessage>(width, height, codec))
                     throw std::runtime_error("FAILED to send format message");
             }
-            if (f_log) {
-                if (log_binary) {
-                    fwrite(frame.buffer, frame.buffer_size, 1, f_log);
-                } else {
-                    fprintf(f_log, "%" PRIu64 ": Frame of %zu bytes:\n",
-                            get_time(), frame.buffer_size);
-                    hexdump(frame.buffer, frame.buffer_size, f_log);
-                }
+            if (frame_log) {
+                frame_log.dump(frame.buffer, frame.buffer_size);
             }
             if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size)) {
                 syslog(LOG_ERR, "FAILED to send a frame\n");
@@ -526,6 +560,7 @@  int main(int argc, char* argv[])
     const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
     char opt;
     const char *log_filename = NULL;
+    bool log_binary = false;
     int logmask = LOG_UPTO(LOG_WARNING);
     struct option long_options[] = {
         { "log-binary", no_argument, NULL, 'b'},
@@ -538,7 +573,7 @@  int main(int argc, char* argv[])
 
     setlogmask(logmask);
 
-    while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL)) != -1) {
+    while ((opt = getopt_long(argc, argv, "bhp:c:l:d", long_options, NULL)) != -1) {
         switch (opt) {
         case 0:
             /* Handle long options if needed */
@@ -579,31 +614,17 @@  int main(int argc, char* argv[])
 
     register_interrupts();
 
-    FILE *f_log = NULL;
-    if (log_filename) {
-        f_log = fopen(log_filename, "wb");
-        if (!f_log) {
-            syslog(LOG_ERR, "Failed to open log file '%s': %s\n",
-                   log_filename, strerror(errno));
-            return EXIT_FAILURE;
-        }
-    }
-
     int ret = EXIT_SUCCESS;
     try {
         Stream streamfd(streamport);
         X11CursorThread cursor_thread(streamfd);
-        do_capture(streamfd, streamport, f_log);
+        FrameLog frame_log(log_filename, log_binary);
+        do_capture(streamfd, streamport, frame_log);
     }
     catch (std::runtime_error &err) {
         syslog(LOG_ERR, "%s\n", err.what());
         ret = EXIT_FAILURE;
     }
-
-    if (f_log) {
-        fclose(f_log);
-        f_log = NULL;
-    }
     closelog();
     return ret;
 }

Comments

> 
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 99
>  ++++++++++++++++++++++++++-----------------
>  1 file changed, 60 insertions(+), 39 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 1c7c42d..2c38233 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -48,6 +48,17 @@ namespace spice
>  namespace streaming_agent
>  {
>  
> +/* returns current time in micro-seconds */
> +static uint64_t get_time(void)
> +{
> +    struct timeval now;
> +
> +    gettimeofday(&now, NULL);
> +
> +    return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
> +
> +}
> +
>  class Stream
>  {
>  public:

Is this move required?
I would remove the "void" in the argument and the extra empty line after the return.

> @@ -149,6 +160,7 @@ struct FrameMessage : Message<StreamMsgData,
> FrameMessage>
>      }
>  };
>  
> +
>  struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
>  {
>      X11CursorMessage(XFixesCursorImage *cursor)

This should go in the patch introducing this extra line

> @@ -198,6 +210,46 @@ private:
>  };
>  
>  
> +class FrameLog
> +{
> +public:
> +    FrameLog(const char *filename, bool binary = false);
> +    ~FrameLog();
> +
> +    operator bool() { return log != NULL; }
> +    void dump(const void *buffer, size_t length);
> +

I would remove the operator bool and just call dump, maybe if
you are concerned about speed I would do something like

    void ALWAYS_INLINE dump(const void *buffer, size_t length)
    {
        if (UNLIKELY(log)) {
           real_dump(buffer, length);
        }
    }

but I don't think is necessary.

> +private:
> +    FILE *log;
> +    bool binary;
> +};
> +
> +

There are a lot of change between 1 and 2 empty lines in this series,
I would go for only an empty line.

> +FrameLog::FrameLog(const char *filename, bool binary)
> +    : log(filename ? fopen(filename, "wb") : NULL), binary(binary)
> +{
> +    if (filename && !log) {
> +        // We do not abort the program in that case, it's only a warning
> +        syslog(LOG_WARNING, "Failed to open hexdump log file '%s': %m\n",
> filename);

You are changing the behaviour of the program here, this is not what the
commit message says. In my opinion if the user explicitly decided to have
a log an error creating it should be a failure (as was before).

> +    }
> +}
> +
> +FrameLog::~FrameLog()
> +{
> +    if (log)
> +        fclose(log);

style

> +}
> +
> +void FrameLog::dump(const void *buffer, size_t length)
> +{
> +    if (binary) {
> +        fwrite(buffer, length, 1, log);
> +    } else {
> +        fprintf(log, "%" PRIu64 ": Frame of %zu bytes:\n", get_time(),
> length);
> +        hexdump(buffer, length, log);
> +    }
> +}
> +
>  class X11CursorThread
>  {
>  public:
> @@ -266,7 +318,6 @@ void X11CursorThread::cursor_changes()
>  
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
> -static bool log_binary = false;
>  static std::set<SpiceVideoCodecType> client_codecs;
>  
>  int Stream::have_something_to_read(int timeout)
> @@ -401,17 +452,6 @@ size_t Stream::write_all(const char *what, const void
> *buf, const size_t len)
>      return written;
>  }
>  
> -/* returns current time in micro-seconds */
> -static uint64_t get_time(void)
> -{
> -    struct timeval now;
> -
> -    gettimeofday(&now, NULL);
> -
> -    return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
> -
> -}
> -
>  static void handle_interrupt(int intr)
>  {
>      syslog(LOG_INFO, "Got signal %d, exiting", intr);
> @@ -434,7 +474,7 @@ static void usage(const char *progname)
>      printf("options are:\n");
>      printf("\t-p portname  -- virtio-serial port to use\n");
>      printf("\t-l file -- log frames to file\n");
> -    printf("\t--log-binary -- log binary frames (following -l)\n");
> +    printf("\t-b or --log-binary -- log binary frames (following -l)\n");
>      printf("\t-d -- enable debug logs\n");
>      printf("\t-c variable=value -- change settings\n");
>      printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");

This is another change not in the commit log.
I personally disagree (as already pointed out) with this change.

> @@ -445,7 +485,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -do_capture(Stream &stream, const char *streamport, FILE *f_log)
> +do_capture(Stream &stream, const char *streamport, FrameLog &frame_log)
>  {
>      unsigned int frame_count = 0;
>      while (!quit_requested) {
> @@ -497,14 +537,8 @@ do_capture(Stream &stream, const char *streamport, FILE
> *f_log)
>                  if (!stream.send<FormatMessage>(width, height, codec))
>                      throw std::runtime_error("FAILED to send format
>                      message");
>              }
> -            if (f_log) {
> -                if (log_binary) {
> -                    fwrite(frame.buffer, frame.buffer_size, 1, f_log);
> -                } else {
> -                    fprintf(f_log, "%" PRIu64 ": Frame of %zu bytes:\n",
> -                            get_time(), frame.buffer_size);
> -                    hexdump(frame.buffer, frame.buffer_size, f_log);
> -                }
> +            if (frame_log) {
> +                frame_log.dump(frame.buffer, frame.buffer_size);
>              }
>              if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size))
>              {
>                  syslog(LOG_ERR, "FAILED to send a frame\n");
> @@ -526,6 +560,7 @@ int main(int argc, char* argv[])
>      const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
>      char opt;
>      const char *log_filename = NULL;
> +    bool log_binary = false;
>      int logmask = LOG_UPTO(LOG_WARNING);
>      struct option long_options[] = {
>          { "log-binary", no_argument, NULL, 'b'},
> @@ -538,7 +573,7 @@ int main(int argc, char* argv[])
>  
>      setlogmask(logmask);
>  
> -    while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL))
> != -1) {
> +    while ((opt = getopt_long(argc, argv, "bhp:c:l:d", long_options, NULL))
> != -1) {
>          switch (opt) {
>          case 0:
>              /* Handle long options if needed */
> @@ -579,31 +614,17 @@ int main(int argc, char* argv[])
>  
>      register_interrupts();
>  
> -    FILE *f_log = NULL;
> -    if (log_filename) {
> -        f_log = fopen(log_filename, "wb");
> -        if (!f_log) {
> -            syslog(LOG_ERR, "Failed to open log file '%s': %s\n",
> -                   log_filename, strerror(errno));
> -            return EXIT_FAILURE;
> -        }
> -    }
> -
>      int ret = EXIT_SUCCESS;
>      try {
>          Stream streamfd(streamport);
>          X11CursorThread cursor_thread(streamfd);
> -        do_capture(streamfd, streamport, f_log);
> +        FrameLog frame_log(log_filename, log_binary);
> +        do_capture(streamfd, streamport, frame_log);
>      }
>      catch (std::runtime_error &err) {
>          syslog(LOG_ERR, "%s\n", err.what());
>          ret = EXIT_FAILURE;
>      }
> -
> -    if (f_log) {
> -        fclose(f_log);
> -        f_log = NULL;
> -    }
>      closelog();
>      return ret;
>  }

Frediano