[15/22] Create FrameLog class to encapsulate logging of frames

Submitted by Christophe de Dinechin on Feb. 28, 2018, 3:43 p.m.

Details

Message ID 20180228154325.25791-16-christophe@dinechin.org
State New
Headers show
Series "streaming-agent: C++ refactoring" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 28, 2018, 3:43 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

The FrameLog encapsulates the frame binary log, making sure that the
log is properly closed in case of exceptions.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 include/spice-streaming-agent/errors.hpp | 10 ++++
 src/errors.cpp                           |  6 ++
 src/spice-streaming-agent.cpp            | 94 +++++++++++++++++++-------------
 3 files changed, 72 insertions(+), 38 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
index ca70d2e..72e9910 100644
--- a/include/spice-streaming-agent/errors.hpp
+++ b/include/spice-streaming-agent/errors.hpp
@@ -34,6 +34,16 @@  protected:
     int saved_errno;
 };
 
+class OpenError : public IOError
+{
+public:
+    OpenError(const char *msg, const char *filename, int saved_errno)
+        : IOError(msg, saved_errno), filename(filename) {}
+    int format_message(char *buffer, size_t size) const noexcept override;
+protected:
+    const char *filename;
+};
+
 class WriteError : public IOError
 {
 public:
diff --git a/src/errors.cpp b/src/errors.cpp
index 01bb162..ff25142 100644
--- a/src/errors.cpp
+++ b/src/errors.cpp
@@ -47,6 +47,12 @@  int IOError::append_strerror(char *buffer, size_t size, int written) const noexc
     return written;
 }
 
+int OpenError::format_message(char *buffer, size_t size) const noexcept
+{
+    int written = snprintf(buffer, size, "%s '%s'", what(), filename);
+    return append_strerror(buffer, size, written);
+}
+
 int WriteError::format_message(char *buffer, size_t size) const noexcept
 {
     int written = snprintf(buffer, size, "%s writing %s", what(), operation);
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index d1996fd..9e643bf 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
 {
     typedef std::set<SpiceVideoCodecType> codecs_t;
@@ -218,6 +229,45 @@  private:
     Display *display;
 };
 
+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) {
+        throw OpenError("failed to open hexdump log file", filename, errno);
+    }
+}
+
+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:
@@ -279,11 +329,9 @@  X11CursorThread::X11CursorThread(Stream &stream)
     thread.detach();
 }
 
-
 }} // namespace spice::streaming_agent
 
 static bool quit_requested = false;
-static bool log_binary = false;
 
 int Stream::have_something_to_read(int timeout)
 {
@@ -407,17 +455,6 @@  void Stream::write_all(const char *operation, const void *buf, const size_t len)
     syslog(LOG_DEBUG, "write_all -- %zu bytes written\n", 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);
@@ -452,7 +489,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) {
@@ -504,14 +541,8 @@  do_capture(Stream &stream, const char *streamport, FILE *f_log)
 
                 stream.send<FormatMessage>(width, height, codec);
             }
-            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);
             }
             stream.send<FrameMessage>(frame.buffer, frame.buffer_size);
             //usleep(1);
@@ -530,6 +561,7 @@  int main(int argc, char* argv[])
     const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
     int opt;
     const char *log_filename = NULL;
+    bool log_binary = false;
     int logmask = LOG_UPTO(LOG_WARNING);
     const char *pluginsdir = PLUGINSDIR;
     enum {
@@ -593,21 +625,12 @@  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 stream(streamport);
+        FrameLog frame_log(log_filename, log_binary);
         X11CursorThread cursor_thread(stream);
-        do_capture(stream, streamport, f_log);
+        do_capture(stream, streamport, frame_log);
     }
     catch (Error &err) {
         err.syslog();
@@ -617,11 +640,6 @@  int main(int argc, char* argv[])
         syslog(LOG_ERR, "%s\n", err.what());
         ret = EXIT_FAILURE;
     }
-
-    if (f_log) {
-        fclose(f_log);
-        f_log = NULL;
-    }
     closelog();
     return ret;
 }

Comments

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> The FrameLog encapsulates the frame binary log, making sure that the
> log is properly closed in case of exceptions.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  include/spice-streaming-agent/errors.hpp | 10 ++++
>  src/errors.cpp                           |  6 ++
>  src/spice-streaming-agent.cpp            | 94 +++++++++++++++++++-------------
>  3 files changed, 72 insertions(+), 38 deletions(-)
> 
> diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
> index ca70d2e..72e9910 100644
> --- a/include/spice-streaming-agent/errors.hpp
> +++ b/include/spice-streaming-agent/errors.hpp
> @@ -34,6 +34,16 @@ protected:
>      int saved_errno;
>  };
>  
> +class OpenError : public IOError
> +{
> +public:
> +    OpenError(const char *msg, const char *filename, int saved_errno)
> +        : IOError(msg, saved_errno), filename(filename) {}
> +    int format_message(char *buffer, size_t size) const noexcept override;
> +protected:
> +    const char *filename;
> +};
> +
>  class WriteError : public IOError
>  {
>  public:
> diff --git a/src/errors.cpp b/src/errors.cpp
> index 01bb162..ff25142 100644
> --- a/src/errors.cpp
> +++ b/src/errors.cpp
> @@ -47,6 +47,12 @@ int IOError::append_strerror(char *buffer, size_t size, int written) const noexc
>      return written;
>  }
>  
> +int OpenError::format_message(char *buffer, size_t size) const noexcept
> +{
> +    int written = snprintf(buffer, size, "%s '%s'", what(), filename);
> +    return append_strerror(buffer, size, written);
> +}

This doesn't put the strerror(errno) into the error message?

> +
>  int WriteError::format_message(char *buffer, size_t size) const noexcept
>  {
>      int written = snprintf(buffer, size, "%s writing %s", what(), operation);
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index d1996fd..9e643bf 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
>  {
>      typedef std::set<SpiceVideoCodecType> codecs_t;
> @@ -218,6 +229,45 @@ private:
>      Display *display;
>  };
>  
> +class FrameLog
> +{
> +public:
> +    FrameLog(const char *filename, bool binary = false);
> +    ~FrameLog();
> +
> +    operator bool() { return log != NULL; }

Shall we use nullptr?

> +    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) {
> +        throw OpenError("failed to open hexdump log file", filename, errno);
> +    }
> +}
> +
> +FrameLog::~FrameLog()
> +{
> +    if (log)
> +        fclose(log);

Brackets :)

> +}
> +
> +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);
> +    }

The class allows to pass a nullptr as filename and will initialize the
"log" member to nullptr as well.

I haven't found what fwrite or fprintf (called from hexdump()) will do
if you pass it a nullptr as FILE*. The return codes aren't checked
either, but that was there before, so possibly a followup.

But what use is there to allow to have a FrameLog::log = nullptr?

> +}
> +
>  class X11CursorThread
>  {
>  public:
> @@ -279,11 +329,9 @@ X11CursorThread::X11CursorThread(Stream &stream)
>      thread.detach();
>  }
>  
> -
>  }} // namespace spice::streaming_agent
>  
>  static bool quit_requested = false;
> -static bool log_binary = false;
>  
>  int Stream::have_something_to_read(int timeout)
>  {
> @@ -407,17 +455,6 @@ void Stream::write_all(const char *operation, const void *buf, const size_t len)
>      syslog(LOG_DEBUG, "write_all -- %zu bytes written\n", 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);
> @@ -452,7 +489,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) {
> @@ -504,14 +541,8 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>  
>                  stream.send<FormatMessage>(width, height, codec);
>              }
> -            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);
>              }
>              stream.send<FrameMessage>(frame.buffer, frame.buffer_size);
>              //usleep(1);
> @@ -530,6 +561,7 @@ int main(int argc, char* argv[])
>      const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
>      int opt;
>      const char *log_filename = NULL;
> +    bool log_binary = false;
>      int logmask = LOG_UPTO(LOG_WARNING);
>      const char *pluginsdir = PLUGINSDIR;
>      enum {
> @@ -593,21 +625,12 @@ 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 stream(streamport);
> +        FrameLog frame_log(log_filename, log_binary);
>          X11CursorThread cursor_thread(stream);
> -        do_capture(stream, streamport, f_log);
> +        do_capture(stream, streamport, frame_log);
>      }
>      catch (Error &err) {
>          err.syslog();
> @@ -617,11 +640,6 @@ int main(int argc, char* argv[])
>          syslog(LOG_ERR, "%s\n", err.what());
>          ret = EXIT_FAILURE;
>      }
> -
> -    if (f_log) {
> -        fclose(f_log);
> -        f_log = NULL;
> -    }
>      closelog();
>      return ret;
>  }
> On 1 Mar 2018, at 15:11, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> The FrameLog encapsulates the frame binary log, making sure that the
>> log is properly closed in case of exceptions.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> include/spice-streaming-agent/errors.hpp | 10 ++++
>> src/errors.cpp                           |  6 ++
>> src/spice-streaming-agent.cpp            | 94 +++++++++++++++++++-------------
>> 3 files changed, 72 insertions(+), 38 deletions(-)
>> 
>> diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
>> index ca70d2e..72e9910 100644
>> --- a/include/spice-streaming-agent/errors.hpp
>> +++ b/include/spice-streaming-agent/errors.hpp
>> @@ -34,6 +34,16 @@ protected:
>>     int saved_errno;
>> };
>> 
>> +class OpenError : public IOError
>> +{
>> +public:
>> +    OpenError(const char *msg, const char *filename, int saved_errno)
>> +        : IOError(msg, saved_errno), filename(filename) {}
>> +    int format_message(char *buffer, size_t size) const noexcept override;
>> +protected:
>> +    const char *filename;
>> +};
>> +
>> class WriteError : public IOError
>> {
>> public:
>> diff --git a/src/errors.cpp b/src/errors.cpp
>> index 01bb162..ff25142 100644
>> --- a/src/errors.cpp
>> +++ b/src/errors.cpp
>> @@ -47,6 +47,12 @@ int IOError::append_strerror(char *buffer, size_t size, int written) const noexc
>>     return written;
>> }
>> 
>> +int OpenError::format_message(char *buffer, size_t size) const noexcept
>> +{
>> +    int written = snprintf(buffer, size, "%s '%s'", what(), filename);
>> +    return append_strerror(buffer, size, written);
>> +}
> 
> This doesn't put the strerror(errno) into the error message?

Not sure what you mean? That’s exactly what append_strerror does. Did I miss something?

> 
>> +
>> int WriteError::format_message(char *buffer, size_t size) const noexcept
>> {
>>     int written = snprintf(buffer, size, "%s writing %s", what(), operation);
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index d1996fd..9e643bf 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
>> {
>>     typedef std::set<SpiceVideoCodecType> codecs_t;
>> @@ -218,6 +229,45 @@ private:
>>     Display *display;
>> };
>> 
>> +class FrameLog
>> +{
>> +public:
>> +    FrameLog(const char *filename, bool binary = false);
>> +    ~FrameLog();
>> +
>> +    operator bool() { return log != NULL; }
> 
> Shall we use nullptr?

Why not. But I would recommend a cleanup that replaces all NULL with nullptr.

> 
>> +    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) {
>> +        throw OpenError("failed to open hexdump log file", filename, errno);
>> +    }
>> +}
>> +
>> +FrameLog::~FrameLog()
>> +{
>> +    if (log)
>> +        fclose(log);
> 
> Brackets :)

Aaargh. I missed that one. Fixed…

> 
>> +}
>> +
>> +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);
>> +    }
> 
> The class allows to pass a nullptr as filename and will initialize the
> "log" member to nullptr as well.
> 
> I haven't found what fwrite or fprintf (called from hexdump()) will do
> if you pass it a nullptr as FILE*. The return codes aren't checked
> either, but that was there before, so possibly a followup.
> 
> But what use is there to allow to have a FrameLog::log = nullptr?

I stuck with the existing pattern that was testing frame_log before calling dump.
But I can put the test inside. I now remember that someone suggested that, I believe (don’t remember who).

> 
>> +}
>> +
>> class X11CursorThread
>> {
>> public:
>> @@ -279,11 +329,9 @@ X11CursorThread::X11CursorThread(Stream &stream)
>>     thread.detach();
>> }
>> 
>> -
>> }} // namespace spice::streaming_agent
>> 
>> static bool quit_requested = false;
>> -static bool log_binary = false;
>> 
>> int Stream::have_something_to_read(int timeout)
>> {
>> @@ -407,17 +455,6 @@ void Stream::write_all(const char *operation, const void *buf, const size_t len)
>>     syslog(LOG_DEBUG, "write_all -- %zu bytes written\n", 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);
>> @@ -452,7 +489,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) {
>> @@ -504,14 +541,8 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>> 
>>                 stream.send<FormatMessage>(width, height, codec);
>>             }
>> -            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);
>>             }
>>             stream.send<FrameMessage>(frame.buffer, frame.buffer_size);
>>             //usleep(1);
>> @@ -530,6 +561,7 @@ int main(int argc, char* argv[])
>>     const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
>>     int opt;
>>     const char *log_filename = NULL;
>> +    bool log_binary = false;
>>     int logmask = LOG_UPTO(LOG_WARNING);
>>     const char *pluginsdir = PLUGINSDIR;
>>     enum {
>> @@ -593,21 +625,12 @@ 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 stream(streamport);
>> +        FrameLog frame_log(log_filename, log_binary);
>>         X11CursorThread cursor_thread(stream);
>> -        do_capture(stream, streamport, f_log);
>> +        do_capture(stream, streamport, frame_log);
>>     }
>>     catch (Error &err) {
>>         err.syslog();
>> @@ -617,11 +640,6 @@ int main(int argc, char* argv[])
>>         syslog(LOG_ERR, "%s\n", err.what());
>>         ret = EXIT_FAILURE;
>>     }
>> -
>> -    if (f_log) {
>> -        fclose(f_log);
>> -        f_log = NULL;
>> -    }
>>     closelog();
>>     return ret;
>> }
On Thu, 2018-03-01 at 15:42 +0100, Christophe de Dinechin wrote:
> > On 1 Mar 2018, at 15:11, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > The FrameLog encapsulates the frame binary log, making sure that the
> > > log is properly closed in case of exceptions.
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > > include/spice-streaming-agent/errors.hpp | 10 ++++
> > > src/errors.cpp                           |  6 ++
> > > src/spice-streaming-agent.cpp            | 94 +++++++++++++++++++-------------
> > > 3 files changed, 72 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
> > > index ca70d2e..72e9910 100644
> > > --- a/include/spice-streaming-agent/errors.hpp
> > > +++ b/include/spice-streaming-agent/errors.hpp
> > > @@ -34,6 +34,16 @@ protected:
> > >     int saved_errno;
> > > };
> > > 
> > > +class OpenError : public IOError
> > > +{
> > > +public:
> > > +    OpenError(const char *msg, const char *filename, int saved_errno)
> > > +        : IOError(msg, saved_errno), filename(filename) {}
> > > +    int format_message(char *buffer, size_t size) const noexcept override;
> > > +protected:
> > > +    const char *filename;
> > > +};
> > > +
> > > class WriteError : public IOError
> > > {
> > > public:
> > > diff --git a/src/errors.cpp b/src/errors.cpp
> > > index 01bb162..ff25142 100644
> > > --- a/src/errors.cpp
> > > +++ b/src/errors.cpp
> > > @@ -47,6 +47,12 @@ int IOError::append_strerror(char *buffer, size_t size, int written) const noexc
> > >     return written;
> > > }
> > > 
> > > +int OpenError::format_message(char *buffer, size_t size) const noexcept
> > > +{
> > > +    int written = snprintf(buffer, size, "%s '%s'", what(), filename);
> > > +    return append_strerror(buffer, size, written);
> > > +}
> > 
> > This doesn't put the strerror(errno) into the error message?
> 
> Not sure what you mean? That’s exactly what append_strerror does. Did I miss something?

Oh, right, it was me who somehow missed that, sorry.

> > 
> > > +
> > > int WriteError::format_message(char *buffer, size_t size) const noexcept
> > > {
> > >     int written = snprintf(buffer, size, "%s writing %s", what(), operation);
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index d1996fd..9e643bf 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
> > > {
> > >     typedef std::set<SpiceVideoCodecType> codecs_t;
> > > @@ -218,6 +229,45 @@ private:
> > >     Display *display;
> > > };
> > > 
> > > +class FrameLog
> > > +{
> > > +public:
> > > +    FrameLog(const char *filename, bool binary = false);
> > > +    ~FrameLog();
> > > +
> > > +    operator bool() { return log != NULL; }
> > 
> > Shall we use nullptr?
> 
> Why not. But I would recommend a cleanup that replaces all NULL with nullptr.
> 
> > 
> > > +    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) {
> > > +        throw OpenError("failed to open hexdump log file", filename, errno);
> > > +    }
> > > +}
> > > +
> > > +FrameLog::~FrameLog()
> > > +{
> > > +    if (log)
> > > +        fclose(log);
> > 
> > Brackets :)
> 
> Aaargh. I missed that one. Fixed…
> 
> > 
> > > +}
> > > +
> > > +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);
> > > +    }
> > 
> > The class allows to pass a nullptr as filename and will initialize the
> > "log" member to nullptr as well.
> > 
> > I haven't found what fwrite or fprintf (called from hexdump()) will do
> > if you pass it a nullptr as FILE*. The return codes aren't checked
> > either, but that was there before, so possibly a followup.
> > 
> > But what use is there to allow to have a FrameLog::log = nullptr?
> 
> I stuck with the existing pattern that was testing frame_log before calling dump.
> But I can put the test inside. I now remember that someone suggested that, I believe (don’t remember who).

Right, my bad as well (seems I need a coffee - well, in my case, tea
:)), I've missed that too. Yeah, in that case, putting the test inside
is probably the way to go, to ensure the class is used correctly (and
save a few LOCs).

> 
> > 
> > > +}
> > > +
> > > class X11CursorThread
> > > {
> > > public:
> > > @@ -279,11 +329,9 @@ X11CursorThread::X11CursorThread(Stream &stream)
> > >     thread.detach();
> > > }
> > > 
> > > -
> > > }} // namespace spice::streaming_agent
> > > 
> > > static bool quit_requested = false;
> > > -static bool log_binary = false;
> > > 
> > > int Stream::have_something_to_read(int timeout)
> > > {
> > > @@ -407,17 +455,6 @@ void Stream::write_all(const char *operation, const void *buf, const size_t len)
> > >     syslog(LOG_DEBUG, "write_all -- %zu bytes written\n", 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);
> > > @@ -452,7 +489,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) {
> > > @@ -504,14 +541,8 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
> > > 
> > >                 stream.send<FormatMessage>(width, height, codec);
> > >             }
> > > -            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);
> > >             }
> > >             stream.send<FrameMessage>(frame.buffer, frame.buffer_size);
> > >             //usleep(1);
> > > @@ -530,6 +561,7 @@ int main(int argc, char* argv[])
> > >     const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
> > >     int opt;
> > >     const char *log_filename = NULL;
> > > +    bool log_binary = false;
> > >     int logmask = LOG_UPTO(LOG_WARNING);
> > >     const char *pluginsdir = PLUGINSDIR;
> > >     enum {
> > > @@ -593,21 +625,12 @@ 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 stream(streamport);
> > > +        FrameLog frame_log(log_filename, log_binary);
> > >         X11CursorThread cursor_thread(stream);
> > > -        do_capture(stream, streamport, f_log);
> > > +        do_capture(stream, streamport, frame_log);
> > >     }
> > >     catch (Error &err) {
> > >         err.syslog();
> > > @@ -617,11 +640,6 @@ int main(int argc, char* argv[])
> > >         syslog(LOG_ERR, "%s\n", err.what());
> > >         ret = EXIT_FAILURE;
> > >     }
> > > -
> > > -    if (f_log) {
> > > -        fclose(f_log);
> > > -        f_log = NULL;
> > > -    }
> > >     closelog();
> > >     return ret;
> > > }
> 
>
> On 1 Mar 2018, at 15:54, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Thu, 2018-03-01 at 15:42 +0100, Christophe de Dinechin wrote:
>>> On 1 Mar 2018, at 15:11, Lukáš Hrázký <lhrazky@redhat.com> wrote:
>>> 
>>> On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>> 
>>>> The FrameLog encapsulates the frame binary log, making sure that the
>>>> log is properly closed in case of exceptions.
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>> ---
>>>> include/spice-streaming-agent/errors.hpp | 10 ++++
>>>> src/errors.cpp                           |  6 ++
>>>> src/spice-streaming-agent.cpp            | 94 +++++++++++++++++++-------------
>>>> 3 files changed, 72 insertions(+), 38 deletions(-)
>>>> 
>>>> diff --git a/include/spice-streaming-agent/errors.hpp b/include/spice-streaming-agent/errors.hpp
>>>> index ca70d2e..72e9910 100644
>>>> --- a/include/spice-streaming-agent/errors.hpp
>>>> +++ b/include/spice-streaming-agent/errors.hpp
>>>> @@ -34,6 +34,16 @@ protected:
>>>>    int saved_errno;
>>>> };
>>>> 
>>>> +class OpenError : public IOError
>>>> +{
>>>> +public:
>>>> +    OpenError(const char *msg, const char *filename, int saved_errno)
>>>> +        : IOError(msg, saved_errno), filename(filename) {}
>>>> +    int format_message(char *buffer, size_t size) const noexcept override;
>>>> +protected:
>>>> +    const char *filename;
>>>> +};
>>>> +
>>>> class WriteError : public IOError
>>>> {
>>>> public:
>>>> diff --git a/src/errors.cpp b/src/errors.cpp
>>>> index 01bb162..ff25142 100644
>>>> --- a/src/errors.cpp
>>>> +++ b/src/errors.cpp
>>>> @@ -47,6 +47,12 @@ int IOError::append_strerror(char *buffer, size_t size, int written) const noexc
>>>>    return written;
>>>> }
>>>> 
>>>> +int OpenError::format_message(char *buffer, size_t size) const noexcept
>>>> +{
>>>> +    int written = snprintf(buffer, size, "%s '%s'", what(), filename);
>>>> +    return append_strerror(buffer, size, written);
>>>> +}
>>> 
>>> This doesn't put the strerror(errno) into the error message?
>> 
>> Not sure what you mean? That’s exactly what append_strerror does. Did I miss something?
> 
> Oh, right, it was me who somehow missed that, sorry.
> 
>>> 
>>>> +
>>>> int WriteError::format_message(char *buffer, size_t size) const noexcept
>>>> {
>>>>    int written = snprintf(buffer, size, "%s writing %s", what(), operation);
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index d1996fd..9e643bf 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
>>>> {
>>>>    typedef std::set<SpiceVideoCodecType> codecs_t;
>>>> @@ -218,6 +229,45 @@ private:
>>>>    Display *display;
>>>> };
>>>> 
>>>> +class FrameLog
>>>> +{
>>>> +public:
>>>> +    FrameLog(const char *filename, bool binary = false);
>>>> +    ~FrameLog();
>>>> +
>>>> +    operator bool() { return log != NULL; }
>>> 
>>> Shall we use nullptr?
>> 
>> Why not. But I would recommend a cleanup that replaces all NULL with nullptr.
>> 
>>> 
>>>> +    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) {
>>>> +        throw OpenError("failed to open hexdump log file", filename, errno);
>>>> +    }
>>>> +}
>>>> +
>>>> +FrameLog::~FrameLog()
>>>> +{
>>>> +    if (log)
>>>> +        fclose(log);
>>> 
>>> Brackets :)
>> 
>> Aaargh. I missed that one. Fixed…
>> 
>>> 
>>>> +}
>>>> +
>>>> +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);
>>>> +    }
>>> 
>>> The class allows to pass a nullptr as filename and will initialize the
>>> "log" member to nullptr as well.
>>> 
>>> I haven't found what fwrite or fprintf (called from hexdump()) will do
>>> if you pass it a nullptr as FILE*. The return codes aren't checked
>>> either, but that was there before, so possibly a followup.
>>> 
>>> But what use is there to allow to have a FrameLog::log = nullptr?
>> 
>> I stuck with the existing pattern that was testing frame_log before calling dump.
>> But I can put the test inside. I now remember that someone suggested that, I believe (don’t remember who).
> 
> Right, my bad as well (seems I need a coffee - well, in my case, tea
> :)), I've missed that too. Yeah, in that case, putting the test inside
> is probably the way to go, to ensure the class is used correctly (and
> save a few LOCs).

Done.

> 
>> 
>>> 
>>>> +}
>>>> +
>>>> class X11CursorThread
>>>> {
>>>> public:
>>>> @@ -279,11 +329,9 @@ X11CursorThread::X11CursorThread(Stream &stream)
>>>>    thread.detach();
>>>> }
>>>> 
>>>> -
>>>> }} // namespace spice::streaming_agent
>>>> 
>>>> static bool quit_requested = false;
>>>> -static bool log_binary = false;
>>>> 
>>>> int Stream::have_something_to_read(int timeout)
>>>> {
>>>> @@ -407,17 +455,6 @@ void Stream::write_all(const char *operation, const void *buf, const size_t len)
>>>>    syslog(LOG_DEBUG, "write_all -- %zu bytes written\n", 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);
>>>> @@ -452,7 +489,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) {
>>>> @@ -504,14 +541,8 @@ do_capture(Stream &stream, const char *streamport, FILE *f_log)
>>>> 
>>>>                stream.send<FormatMessage>(width, height, codec);
>>>>            }
>>>> -            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);
>>>>            }
>>>>            stream.send<FrameMessage>(frame.buffer, frame.buffer_size);
>>>>            //usleep(1);
>>>> @@ -530,6 +561,7 @@ int main(int argc, char* argv[])
>>>>    const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
>>>>    int opt;
>>>>    const char *log_filename = NULL;
>>>> +    bool log_binary = false;
>>>>    int logmask = LOG_UPTO(LOG_WARNING);
>>>>    const char *pluginsdir = PLUGINSDIR;
>>>>    enum {
>>>> @@ -593,21 +625,12 @@ 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 stream(streamport);
>>>> +        FrameLog frame_log(log_filename, log_binary);
>>>>        X11CursorThread cursor_thread(stream);
>>>> -        do_capture(stream, streamport, f_log);
>>>> +        do_capture(stream, streamport, frame_log);
>>>>    }
>>>>    catch (Error &err) {
>>>>        err.syslog();
>>>> @@ -617,11 +640,6 @@ int main(int argc, char* argv[])
>>>>        syslog(LOG_ERR, "%s\n", err.what());
>>>>        ret = EXIT_FAILURE;
>>>>    }
>>>> -
>>>> -    if (f_log) {
>>>> -        fclose(f_log);
>>>> -        f_log = NULL;
>>>> -    }
>>>>    closelog();
>>>>    return ret;
>>>> }
>> 
>> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel