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