On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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;
> }
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel