[21/22] Moving FrameLog into a separate file

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

Details

Message ID 20180228154325.25791-22-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>

Isolating classes in separate files makes parallel builds faster,
facilitates code reuse and minimizes the chances of patch conflicts.

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/Makefile.am               |  2 ++
 src/frame-log.cpp             | 45 +++++++++++++++++++++++++++++++++++++
 src/frame-log.hpp             | 43 +++++++++++++++++++++++++++++++++++
 src/spice-streaming-agent.cpp | 52 +------------------------------------------
 4 files changed, 91 insertions(+), 51 deletions(-)
 create mode 100644 src/frame-log.cpp
 create mode 100644 src/frame-log.hpp

Patch hide | download patch | download mbox

diff --git a/src/Makefile.am b/src/Makefile.am
index 4a03e5e..5e36e1f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -57,6 +57,8 @@  spice_streaming_agent_SOURCES = \
 	jpeg.hpp \
 	stream.cpp \
 	stream.hpp \
+	frame-log.hpp \
+	frame-log.cpp \
 	errors.cpp \
 	x11-cursor.hpp \
 	x11-cursor.cpp \
diff --git a/src/frame-log.cpp b/src/frame-log.cpp
new file mode 100644
index 0000000..53751be
--- /dev/null
+++ b/src/frame-log.cpp
@@ -0,0 +1,45 @@ 
+/* Class to log frames as they are being streamed
+ *
+ * \copyright
+ * Copyright 2018 Red Hat Inc. All rights reserved.
+ */
+
+#include "frame-log.hpp"
+#include "hexdump.h"
+
+#include <spice-streaming-agent/errors.hpp>
+
+#include <syslog.h>
+#include <inttypes.h>
+#include <errno.h>
+
+namespace spice
+{
+namespace streaming_agent
+{
+
+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);
+    }
+}
+
+}} // namespace spice::streaming_agent
diff --git a/src/frame-log.hpp b/src/frame-log.hpp
new file mode 100644
index 0000000..09dbd4a
--- /dev/null
+++ b/src/frame-log.hpp
@@ -0,0 +1,43 @@ 
+/* Class to log frames as they are being streamed
+ *
+ * \copyright
+ * Copyright 2018 Red Hat Inc. All rights reserved.
+ */
+#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
+#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
+
+#include <stdio.h>
+#include <stdint.h>
+#include <sys/time.h>
+
+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 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;
+};
+
+}} // namespace spice::streaming_agent
+
+#endif // SPICE_STREAMING_AGENT_ERRORS_HPP
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 2264af2..424db95 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -7,8 +7,8 @@ 
 #include "concrete-agent.hpp"
 #include "stream.hpp"
 #include "message.hpp"
+#include "frame-log.hpp"
 #include "x11-cursor.hpp"
-#include "hexdump.h"
 #include "mjpeg-fallback.hpp"
 
 #include <spice/stream-device.h>
@@ -45,17 +45,6 @@  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 FormatMessage : public Message<StreamMsgFormat, FormatMessage, STREAM_TYPE_FORMAT>
 {
 public:
@@ -85,45 +74,6 @@  public:
     }
 };
 
-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);
-    }
-}
-
 }} // namespace spice::streaming_agent
 
 bool quit_requested = false;

Comments

On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Isolating classes in separate files makes parallel builds faster,
> facilitates code reuse and minimizes the chances of patch conflicts.
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/Makefile.am               |  2 ++
>  src/frame-log.cpp             | 45 +++++++++++++++++++++++++++++++++++++
>  src/frame-log.hpp             | 43 +++++++++++++++++++++++++++++++++++
>  src/spice-streaming-agent.cpp | 52 +------------------------------------------
>  4 files changed, 91 insertions(+), 51 deletions(-)
>  create mode 100644 src/frame-log.cpp
>  create mode 100644 src/frame-log.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 4a03e5e..5e36e1f 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -57,6 +57,8 @@ spice_streaming_agent_SOURCES = \
>  	jpeg.hpp \
>  	stream.cpp \
>  	stream.hpp \
> +	frame-log.hpp \
> +	frame-log.cpp \
>  	errors.cpp \
>  	x11-cursor.hpp \
>  	x11-cursor.cpp \
> diff --git a/src/frame-log.cpp b/src/frame-log.cpp
> new file mode 100644
> index 0000000..53751be
> --- /dev/null
> +++ b/src/frame-log.cpp
> @@ -0,0 +1,45 @@
> +/* Class to log frames as they are being streamed
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include "frame-log.hpp"
> +#include "hexdump.h"
> +
> +#include <spice-streaming-agent/errors.hpp>
> +
> +#include <syslog.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +
> +namespace spice
> +{
> +namespace streaming_agent
> +{
> +
> +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);
> +    }
> +}
> +
> +}} // namespace spice::streaming_agent
> diff --git a/src/frame-log.hpp b/src/frame-log.hpp
> new file mode 100644
> index 0000000..09dbd4a
> --- /dev/null
> +++ b/src/frame-log.hpp
> @@ -0,0 +1,43 @@
> +/* Class to log frames as they are being streamed
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> +#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <sys/time.h>
> +
> +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;
> +
> +}

Why in the header?

> +
> +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;
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_ERRORS_HPP
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 2264af2..424db95 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -7,8 +7,8 @@
>  #include "concrete-agent.hpp"
>  #include "stream.hpp"
>  #include "message.hpp"
> +#include "frame-log.hpp"
>  #include "x11-cursor.hpp"
> -#include "hexdump.h"
>  #include "mjpeg-fallback.hpp"
>  
>  #include <spice/stream-device.h>
> @@ -45,17 +45,6 @@ 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 FormatMessage : public Message<StreamMsgFormat, FormatMessage, STREAM_TYPE_FORMAT>
>  {
>  public:
> @@ -85,45 +74,6 @@ public:
>      }
>  };
>  
> -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);
> -    }
> -}
> -
>  }} // namespace spice::streaming_agent
>  
>  bool quit_requested = false;
> On 1 Mar 2018, at 16:14, 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>
>> 
>> Isolating classes in separate files makes parallel builds faster,
>> facilitates code reuse and minimizes the chances of patch conflicts.
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/Makefile.am               |  2 ++
>> src/frame-log.cpp             | 45 +++++++++++++++++++++++++++++++++++++
>> src/frame-log.hpp             | 43 +++++++++++++++++++++++++++++++++++
>> src/spice-streaming-agent.cpp | 52 +------------------------------------------
>> 4 files changed, 91 insertions(+), 51 deletions(-)
>> create mode 100644 src/frame-log.cpp
>> create mode 100644 src/frame-log.hpp
>> 
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 4a03e5e..5e36e1f 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -57,6 +57,8 @@ spice_streaming_agent_SOURCES = \
>> 	jpeg.hpp \
>> 	stream.cpp \
>> 	stream.hpp \
>> +	frame-log.hpp \
>> +	frame-log.cpp \
>> 	errors.cpp \
>> 	x11-cursor.hpp \
>> 	x11-cursor.cpp \
>> diff --git a/src/frame-log.cpp b/src/frame-log.cpp
>> new file mode 100644
>> index 0000000..53751be
>> --- /dev/null
>> +++ b/src/frame-log.cpp
>> @@ -0,0 +1,45 @@
>> +/* Class to log frames as they are being streamed
>> + *
>> + * \copyright
>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>> + */
>> +
>> +#include "frame-log.hpp"
>> +#include "hexdump.h"
>> +
>> +#include <spice-streaming-agent/errors.hpp>
>> +
>> +#include <syslog.h>
>> +#include <inttypes.h>
>> +#include <errno.h>
>> +
>> +namespace spice
>> +{
>> +namespace streaming_agent
>> +{
>> +
>> +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);
>> +    }
>> +}
>> +
>> +}} // namespace spice::streaming_agent
>> diff --git a/src/frame-log.hpp b/src/frame-log.hpp
>> new file mode 100644
>> index 0000000..09dbd4a
>> --- /dev/null
>> +++ b/src/frame-log.hpp
>> @@ -0,0 +1,43 @@
>> +/* Class to log frames as they are being streamed
>> + *
>> + * \copyright
>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>> + */
>> +#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
>> +#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
>> +
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <sys/time.h>
>> +
>> +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;
>> +
>> +}
> 
> Why in the header?

After splitting, it’s used in two locations that are in different .cpp files.

> 
>> +
>> +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;
>> +};
>> +
>> +}} // namespace spice::streaming_agent
>> +
>> +#endif // SPICE_STREAMING_AGENT_ERRORS_HPP
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 2264af2..424db95 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -7,8 +7,8 @@
>> #include "concrete-agent.hpp"
>> #include "stream.hpp"
>> #include "message.hpp"
>> +#include "frame-log.hpp"
>> #include "x11-cursor.hpp"
>> -#include "hexdump.h"
>> #include "mjpeg-fallback.hpp"
>> 
>> #include <spice/stream-device.h>
>> @@ -45,17 +45,6 @@ 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 FormatMessage : public Message<StreamMsgFormat, FormatMessage, STREAM_TYPE_FORMAT>
>> {
>> public:
>> @@ -85,45 +74,6 @@ public:
>>     }
>> };
>> 
>> -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);
>> -    }
>> -}
>> -
>> }} // namespace spice::streaming_agent
>> 
>> bool quit_requested = false;
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, 2018-03-01 at 16:47 +0100, Christophe de Dinechin wrote:
> > On 1 Mar 2018, at 16:14, 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>
> > > 
> > > Isolating classes in separate files makes parallel builds faster,
> > > facilitates code reuse and minimizes the chances of patch conflicts.
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > > src/Makefile.am               |  2 ++
> > > src/frame-log.cpp             | 45 +++++++++++++++++++++++++++++++++++++
> > > src/frame-log.hpp             | 43 +++++++++++++++++++++++++++++++++++
> > > src/spice-streaming-agent.cpp | 52 +------------------------------------------
> > > 4 files changed, 91 insertions(+), 51 deletions(-)
> > > create mode 100644 src/frame-log.cpp
> > > create mode 100644 src/frame-log.hpp
> > > 
> > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > index 4a03e5e..5e36e1f 100644
> > > --- a/src/Makefile.am
> > > +++ b/src/Makefile.am
> > > @@ -57,6 +57,8 @@ spice_streaming_agent_SOURCES = \
> > > 	jpeg.hpp \
> > > 	stream.cpp \
> > > 	stream.hpp \
> > > +	frame-log.hpp \
> > > +	frame-log.cpp \
> > > 	errors.cpp \
> > > 	x11-cursor.hpp \
> > > 	x11-cursor.cpp \
> > > diff --git a/src/frame-log.cpp b/src/frame-log.cpp
> > > new file mode 100644
> > > index 0000000..53751be
> > > --- /dev/null
> > > +++ b/src/frame-log.cpp
> > > @@ -0,0 +1,45 @@
> > > +/* Class to log frames as they are being streamed
> > > + *
> > > + * \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +
> > > +#include "frame-log.hpp"
> > > +#include "hexdump.h"
> > > +
> > > +#include <spice-streaming-agent/errors.hpp>
> > > +
> > > +#include <syslog.h>
> > > +#include <inttypes.h>
> > > +#include <errno.h>
> > > +
> > > +namespace spice
> > > +{
> > > +namespace streaming_agent
> > > +{
> > > +
> > > +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);
> > > +    }
> > > +}
> > > +
> > > +}} // namespace spice::streaming_agent
> > > diff --git a/src/frame-log.hpp b/src/frame-log.hpp
> > > new file mode 100644
> > > index 0000000..09dbd4a
> > > --- /dev/null
> > > +++ b/src/frame-log.hpp
> > > @@ -0,0 +1,43 @@
> > > +/* Class to log frames as they are being streamed
> > > + *
> > > + * \copyright
> > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > + */
> > > +#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> > > +#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> > > +
> > > +#include <stdio.h>
> > > +#include <stdint.h>
> > > +#include <sys/time.h>
> > > +
> > > +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;
> > > +
> > > +}
> > 
> > Why in the header?
> 
> After splitting, it’s used in two locations that are in different .cpp files.

Well, the definition should be moved to a .cpp file and it seems to me
then this function belongs into somethings like utils.hpp.

Also (more likely a followup, unless it would spare us creating a
utils.hpp?), std::crono could be used? I need to study it a bit, not
too familiar with it, so I am not sure (Frediano mentioned to me some
problem with that which I forgot).

(I am also fine leaving it as-is and fixing it in a followup
alltogether, as I can see some things snowballing for you and you need
to stop somewhere.)

> > 
> > > +
> > > +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;
> > > +};
> > > +
> > > +}} // namespace spice::streaming_agent
> > > +
> > > +#endif // SPICE_STREAMING_AGENT_ERRORS_HPP
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 2264af2..424db95 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -7,8 +7,8 @@
> > > #include "concrete-agent.hpp"
> > > #include "stream.hpp"
> > > #include "message.hpp"
> > > +#include "frame-log.hpp"
> > > #include "x11-cursor.hpp"
> > > -#include "hexdump.h"
> > > #include "mjpeg-fallback.hpp"
> > > 
> > > #include <spice/stream-device.h>
> > > @@ -45,17 +45,6 @@ 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 FormatMessage : public Message<StreamMsgFormat, FormatMessage, STREAM_TYPE_FORMAT>
> > > {
> > > public:
> > > @@ -85,45 +74,6 @@ public:
> > >     }
> > > };
> > > 
> > > -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);
> > > -    }
> > > -}
> > > -
> > > }} // namespace spice::streaming_agent
> > > 
> > > bool quit_requested = false;
> > 
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
>
> On 2 Mar 2018, at 14:34, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Thu, 2018-03-01 at 16:47 +0100, Christophe de Dinechin wrote:
>>> On 1 Mar 2018, at 16:14, 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>
>>>> 
>>>> Isolating classes in separate files makes parallel builds faster,
>>>> facilitates code reuse and minimizes the chances of patch conflicts.
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>> ---
>>>> src/Makefile.am               |  2 ++
>>>> src/frame-log.cpp             | 45 +++++++++++++++++++++++++++++++++++++
>>>> src/frame-log.hpp             | 43 +++++++++++++++++++++++++++++++++++
>>>> src/spice-streaming-agent.cpp | 52 +------------------------------------------
>>>> 4 files changed, 91 insertions(+), 51 deletions(-)
>>>> create mode 100644 src/frame-log.cpp
>>>> create mode 100644 src/frame-log.hpp
>>>> 
>>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>>> index 4a03e5e..5e36e1f 100644
>>>> --- a/src/Makefile.am
>>>> +++ b/src/Makefile.am
>>>> @@ -57,6 +57,8 @@ spice_streaming_agent_SOURCES = \
>>>> 	jpeg.hpp \
>>>> 	stream.cpp \
>>>> 	stream.hpp \
>>>> +	frame-log.hpp \
>>>> +	frame-log.cpp \
>>>> 	errors.cpp \
>>>> 	x11-cursor.hpp \
>>>> 	x11-cursor.cpp \
>>>> diff --git a/src/frame-log.cpp b/src/frame-log.cpp
>>>> new file mode 100644
>>>> index 0000000..53751be
>>>> --- /dev/null
>>>> +++ b/src/frame-log.cpp
>>>> @@ -0,0 +1,45 @@
>>>> +/* Class to log frames as they are being streamed
>>>> + *
>>>> + * \copyright
>>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>>> + */
>>>> +
>>>> +#include "frame-log.hpp"
>>>> +#include "hexdump.h"
>>>> +
>>>> +#include <spice-streaming-agent/errors.hpp>
>>>> +
>>>> +#include <syslog.h>
>>>> +#include <inttypes.h>
>>>> +#include <errno.h>
>>>> +
>>>> +namespace spice
>>>> +{
>>>> +namespace streaming_agent
>>>> +{
>>>> +
>>>> +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);
>>>> +    }
>>>> +}
>>>> +
>>>> +}} // namespace spice::streaming_agent
>>>> diff --git a/src/frame-log.hpp b/src/frame-log.hpp
>>>> new file mode 100644
>>>> index 0000000..09dbd4a
>>>> --- /dev/null
>>>> +++ b/src/frame-log.hpp
>>>> @@ -0,0 +1,43 @@
>>>> +/* Class to log frames as they are being streamed
>>>> + *
>>>> + * \copyright
>>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>>> + */
>>>> +#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
>>>> +#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
>>>> +
>>>> +#include <stdio.h>
>>>> +#include <stdint.h>
>>>> +#include <sys/time.h>
>>>> +
>>>> +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;
>>>> +
>>>> +}
>>> 
>>> Why in the header?
>> 
>> After splitting, it’s used in two locations that are in different .cpp files.
> 
> Well, the definition should be moved to a .cpp file and it seems to me
> then this function belongs into somethings like utils.hpp.
> 
> Also (more likely a followup, unless it would spare us creating a
> utils.hpp?), std::crono could be used? I need to study it a bit, not
> too familiar with it, so I am not sure (Frediano mentioned to me some
> problem with that which I forgot).

Good idea.

I’m interested in what the problem is for the use cases we have (which is basically logging a time)

> 
> (I am also fine leaving it as-is and fixing it in a followup
> alltogether, as I can see some things snowballing for you and you need
> to stop somewhere.)

Indeed.

> 
>>> 
>>>> +
>>>> +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;
>>>> +};
>>>> +
>>>> +}} // namespace spice::streaming_agent
>>>> +
>>>> +#endif // SPICE_STREAMING_AGENT_ERRORS_HPP
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index 2264af2..424db95 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -7,8 +7,8 @@
>>>> #include "concrete-agent.hpp"
>>>> #include "stream.hpp"
>>>> #include "message.hpp"
>>>> +#include "frame-log.hpp"
>>>> #include "x11-cursor.hpp"
>>>> -#include "hexdump.h"
>>>> #include "mjpeg-fallback.hpp"
>>>> 
>>>> #include <spice/stream-device.h>
>>>> @@ -45,17 +45,6 @@ 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 FormatMessage : public Message<StreamMsgFormat, FormatMessage, STREAM_TYPE_FORMAT>
>>>> {
>>>> public:
>>>> @@ -85,45 +74,6 @@ public:
>>>>    }
>>>> };
>>>> 
>>>> -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);
>>>> -    }
>>>> -}
>>>> -
>>>> }} // namespace spice::streaming_agent
>>>> 
>>>> bool quit_requested = false;
>>> 
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel