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).
>
> >
> > > +}
> > > +
> > > 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