> On 1 Mar 2018, at 15:11, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
>
> 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?
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;
>> }
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel