[Spice-devel] [PATCH spice-streaming-agent v2 1/9] Separate the code for logging frames/times into a class
Frediano Ziglio
fziglio at redhat.com
Wed Jun 27 14:17:50 UTC 2018
>
> The FrameLog class provides RAII for the FILE and encapsulates the
> logging functionality.
>
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
> src/Makefile.am | 2 +
> src/frame-log.cpp | 76 +++++++++++++++++++++++++++++++++++
> src/frame-log.hpp | 39 ++++++++++++++++++
> src/spice-streaming-agent.cpp | 71 ++++++++------------------------
> 4 files changed, 134 insertions(+), 54 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 18ed22c..fead680 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -54,6 +54,8 @@ spice_streaming_agent_SOURCES = \
> concrete-agent.hpp \
> error.cpp \
> error.hpp \
> + frame-log.cpp \
> + frame-log.hpp \
> mjpeg-fallback.cpp \
> mjpeg-fallback.hpp \
> jpeg.cpp \
> diff --git a/src/frame-log.cpp b/src/frame-log.cpp
> new file mode 100644
> index 0000000..b0bd09e
> --- /dev/null
> +++ b/src/frame-log.cpp
> @@ -0,0 +1,76 @@
> +/* A utility logger for logging frames and/or time information.
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include "frame-log.hpp"
> +
> +#include "error.hpp"
> +#include "hexdump.h"
> +
> +#include <cstdarg>
> +#include <string.h>
> +#include <sys/time.h>
> +
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +FrameLog::FrameLog(const char *log_name, bool log_binary, bool log_frames) :
> + log_binary(log_binary),
> + log_frames(log_frames)
> +{
> + if (log_name) {
> + log_file = fopen(log_name, "wb");
> + if (!log_file) {
> + throw Error(std::string("Failed to open log file '") + log_name
> + "': " + strerror(errno));
> + }
> + if (!log_binary) {
> + setlinebuf(log_file);
> + }
> + }
> +}
> +
> +FrameLog::~FrameLog()
> +{
> + if (log_file) {
> + fclose(log_file);
> + }
> +}
> +
> +void FrameLog::log_stat(const char* format, ...)
> +{
> + if (log_file && !log_binary) {
> + fprintf(log_file, "%" PRIu64 ": ", get_time());
> + va_list ap;
> + va_start(ap, format);
> + vfprintf(log_file, format, ap);
> + va_end(ap);
> + fputs("\n", log_file);
Minor, not really important, here fputc can be used instead of fputs.
Note that this code is not doing the same thing of the macros, if the
function is called in multithreaded environment you have to consider
the internal file lock mechanism.
> + }
> +}
> +
> +void FrameLog::log_frame(const void* buffer, size_t buffer_size)
> +{
> + if (log_file) {
> + if (log_binary) {
> + fwrite(buffer, buffer_size, 1, log_file);
> + } else if (log_frames) {
> + hexdump(buffer, buffer_size, log_file);
> + }
> + }
> +}
> +
> +/**
> + * Returns current time in microseconds.
> + */
> +uint64_t FrameLog::get_time()
> +{
> + struct timeval now;
> + gettimeofday(&now, NULL);
> +
> + return (uint64_t)now.tv_sec * 1000000 + (uint64_t)now.tv_usec;
> +}
> +
> +}} // namespace spice::streaming_agent
> diff --git a/src/frame-log.hpp b/src/frame-log.hpp
> new file mode 100644
> index 0000000..8503345
> --- /dev/null
> +++ b/src/frame-log.hpp
> @@ -0,0 +1,39 @@
> +/* A utility logger for logging frames and/or time information.
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#ifndef SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> +#define SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> +
> +#include <cinttypes>
> +#include <stddef.h>
> +#include <stdio.h>
> +
> +
> +namespace spice {
> +namespace streaming_agent {
> +
> +class FrameLog {
> +public:
> + FrameLog(const char *log_name, bool log_binary, bool log_frames);
> + FrameLog(const FrameLog &) = delete;
> + FrameLog &operator=(const FrameLog &) = delete;
> + ~FrameLog();
> +
> + __attribute__ ((format (printf, 2, 3)))
> + void log_stat(const char* format, ...);
> + void log_frame(const void* buffer, size_t buffer_size);
> +
> + static uint64_t get_time();
> +
> +private:
> + FILE *log_file = nullptr;
> + bool log_binary = false;
> + bool log_frames = false;
> +};
> +
> +}} // namespace spice::streaming_agent
> +
> +#endif // SPICE_STREAMING_AGENT_FRAME_LOG_HPP
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 67679ac..11796f3 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -5,8 +5,8 @@
> */
>
> #include "concrete-agent.hpp"
> -#include "hexdump.h"
> #include "mjpeg-fallback.hpp"
> +#include "frame-log.hpp"
> #include "stream-port.hpp"
> #include "error.hpp"
>
> @@ -57,8 +57,6 @@ struct SpiceStreamDataMessage
>
> static bool streaming_requested = false;
> static bool quit_requested = false;
> -static bool log_binary = false;
> -static bool log_frames = false;
> static std::set<SpiceVideoCodecType> client_codecs;
>
> static bool have_something_to_read(StreamPort &stream_port, bool blocking)
> @@ -221,17 +219,6 @@ static void spice_stream_send_frame(StreamPort
> &stream_port, const void *buf, co
> syslog(LOG_DEBUG, "Sent a frame of size %u\n", size);
> }
>
> -/* 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);
> @@ -330,14 +317,8 @@ static void cursor_changes(StreamPort *stream_port,
> Display *display, int event_
> }
> }
>
> -#define STAT_LOG(format, ...) do { \
> - if (f_log && !log_binary) { \
> - fprintf(f_log, "%" PRIu64 ": " format "\n", get_time(), ##
> __VA_ARGS__); \
> - } \
> -} while(0)
> -
> static void
> -do_capture(StreamPort &stream_port, FILE *f_log)
> +do_capture(StreamPort &stream_port, FrameLog &frame_log)
> {
> unsigned int frame_count = 0;
> while (!quit_requested) {
> @@ -361,13 +342,13 @@ do_capture(StreamPort &stream_port, FILE *f_log)
> if (++frame_count % 100 == 0) {
> syslog(LOG_DEBUG, "SENT %d frames\n", frame_count);
> }
> - uint64_t time_before = get_time();
> + uint64_t time_before = FrameLog::get_time();
>
> - STAT_LOG("Capturing...");
> + frame_log.log_stat("Capturing...");
> FrameInfo frame = capture->CaptureFrame();
> - STAT_LOG("Captured");
> + frame_log.log_stat("Captured");
>
> - uint64_t time_after = get_time();
> + uint64_t time_after = FrameLog::get_time();
> syslog(LOG_DEBUG,
> "got a frame -- size is %zu (%" PRIu64 " ms) "
> "(%" PRIu64 " ms from last frame)(%" PRIu64 " us)\n",
> @@ -385,18 +366,12 @@ do_capture(StreamPort &stream_port, FILE *f_log)
> codec = capture->VideoCodecType();
>
> syslog(LOG_DEBUG, "wXh %uX%u codec=%u\n", width, height,
> codec);
> - STAT_LOG("Started new stream wXh %uX%u codec=%u", width,
> height, codec);
> + frame_log.log_stat("Started new stream wXh %uX%u codec=%u",
> width, height, codec);
>
> spice_stream_send_format(stream_port, width, height, codec);
> }
> - STAT_LOG("Frame of %zu bytes:", frame.buffer_size);
> - if (f_log) {
> - if (log_binary) {
> - fwrite(frame.buffer, frame.buffer_size, 1, f_log);
> - } else if (log_frames) {
> - hexdump(frame.buffer, frame.buffer_size, f_log);
> - }
> - }
> + frame_log.log_stat("Frame of %zu bytes:", frame.buffer_size);
OT, follow up. PRIuPTR instead of zu.
> + frame_log.log_frame(frame.buffer, frame.buffer_size);
>
> try {
> spice_stream_send_frame(stream_port, frame.buffer,
> frame.buffer_size);
> @@ -404,7 +379,7 @@ do_capture(StreamPort &stream_port, FILE *f_log)
> syslog(e);
> break;
> }
> - STAT_LOG("Sent");
> + frame_log.log_stat("Sent");
>
> read_command(stream_port, false);
> }
> @@ -416,6 +391,8 @@ int main(int argc, char* argv[])
> const char *stream_port_name =
> "/dev/virtio-ports/org.spice-space.stream.0";
> int opt;
> const char *log_filename = NULL;
> + bool log_binary = false;
> + bool log_frames = false;
> const char *pluginsdir = PLUGINSDIR;
> enum {
> OPT_first = UCHAR_MAX,
> @@ -489,20 +466,10 @@ 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;
> - }
> - if (!log_binary) {
> - setlinebuf(f_log);
> - }
> - for (const std::string& arg: old_args) {
> - STAT_LOG("Args: %s", arg.c_str());
> - }
> + FrameLog frame_log(log_filename, log_binary, log_frames);
> +
OT, maybe future follow up, maybe passing the string got from
command line for categories allows more flexibility?
> + for (const std::string& arg: old_args) {
> + frame_log.log_stat("Args: %s", arg.c_str());
> }
> old_args.clear();
>
> @@ -527,17 +494,13 @@ int main(int argc, char* argv[])
> std::thread cursor_th(cursor_changes, &stream_port, display,
> event_base);
> cursor_th.detach();
>
> - do_capture(stream_port, f_log);
> + do_capture(stream_port, frame_log);
> }
> catch (std::exception &err) {
> syslog(LOG_ERR, "%s\n", err.what());
> ret = EXIT_FAILURE;
> }
>
> - if (f_log) {
> - fclose(f_log);
> - f_log = NULL;
> - }
> closelog();
> return ret;
> }
Otherwise,
Acked-by: Frediano Ziglio <fziglio at redhat.com>
Frediano
More information about the Spice-devel
mailing list