>
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
>
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
> src/spice-streaming-agent.cpp | 99
> ++++++++++++++++++++++++++-----------------
> 1 file changed, 60 insertions(+), 39 deletions(-)
>
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 1c7c42d..2c38233 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
> {
> public:
Is this move required?
I would remove the "void" in the argument and the extra empty line after the return.
> @@ -149,6 +160,7 @@ struct FrameMessage : Message<StreamMsgData,
> FrameMessage>
> }
> };
>
> +
> struct X11CursorMessage : Message<StreamMsgCursorSet, X11CursorMessage>
> {
> X11CursorMessage(XFixesCursorImage *cursor)
This should go in the patch introducing this extra line
> @@ -198,6 +210,46 @@ private:
> };
>
>
> +class FrameLog
> +{
> +public:
> + FrameLog(const char *filename, bool binary = false);
> + ~FrameLog();
> +
> + operator bool() { return log != NULL; }
> + void dump(const void *buffer, size_t length);
> +
I would remove the operator bool and just call dump, maybe if
you are concerned about speed I would do something like
void ALWAYS_INLINE dump(const void *buffer, size_t length)
{
if (UNLIKELY(log)) {
real_dump(buffer, length);
}
}
but I don't think is necessary.
> +private:
> + FILE *log;
> + bool binary;
> +};
> +
> +
There are a lot of change between 1 and 2 empty lines in this series,
I would go for only an empty line.
> +FrameLog::FrameLog(const char *filename, bool binary)
> + : log(filename ? fopen(filename, "wb") : NULL), binary(binary)
> +{
> + if (filename && !log) {
> + // We do not abort the program in that case, it's only a warning
> + syslog(LOG_WARNING, "Failed to open hexdump log file '%s': %m\n",
> filename);
You are changing the behaviour of the program here, this is not what the
commit message says. In my opinion if the user explicitly decided to have
a log an error creating it should be a failure (as was before).
> + }
> +}
> +
> +FrameLog::~FrameLog()
> +{
> + if (log)
> + fclose(log);
style
> +}
> +
> +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);
> + }
> +}
> +
> class X11CursorThread
> {
> public:
> @@ -266,7 +318,6 @@ void X11CursorThread::cursor_changes()
>
> static bool streaming_requested = false;
> static bool quit_requested = false;
> -static bool log_binary = false;
> static std::set<SpiceVideoCodecType> client_codecs;
>
> int Stream::have_something_to_read(int timeout)
> @@ -401,17 +452,6 @@ size_t Stream::write_all(const char *what, const void
> *buf, const size_t len)
> return 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);
> @@ -434,7 +474,7 @@ static void usage(const char *progname)
> printf("options are:\n");
> printf("\t-p portname -- virtio-serial port to use\n");
> printf("\t-l file -- log frames to file\n");
> - printf("\t--log-binary -- log binary frames (following -l)\n");
> + printf("\t-b or --log-binary -- log binary frames (following -l)\n");
> printf("\t-d -- enable debug logs\n");
> printf("\t-c variable=value -- change settings\n");
> printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
This is another change not in the commit log.
I personally disagree (as already pointed out) with this change.
> @@ -445,7 +485,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) {
> @@ -497,14 +537,8 @@ do_capture(Stream &stream, const char *streamport, FILE
> *f_log)
> if (!stream.send<FormatMessage>(width, height, codec))
> throw std::runtime_error("FAILED to send format
> message");
> }
> - 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);
> }
> if (!stream.send<FrameMessage>(frame.buffer, frame.buffer_size))
> {
> syslog(LOG_ERR, "FAILED to send a frame\n");
> @@ -526,6 +560,7 @@ int main(int argc, char* argv[])
> const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
> char opt;
> const char *log_filename = NULL;
> + bool log_binary = false;
> int logmask = LOG_UPTO(LOG_WARNING);
> struct option long_options[] = {
> { "log-binary", no_argument, NULL, 'b'},
> @@ -538,7 +573,7 @@ int main(int argc, char* argv[])
>
> setlogmask(logmask);
>
> - while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL))
> != -1) {
> + while ((opt = getopt_long(argc, argv, "bhp:c:l:d", long_options, NULL))
> != -1) {
> switch (opt) {
> case 0:
> /* Handle long options if needed */
> @@ -579,31 +614,17 @@ 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 streamfd(streamport);
> X11CursorThread cursor_thread(streamfd);
> - do_capture(streamfd, streamport, f_log);
> + FrameLog frame_log(log_filename, log_binary);
> + do_capture(streamfd, streamport, frame_log);
> }
> catch (std::runtime_error &err) {
> syslog(LOG_ERR, "%s\n", err.what());
> ret = EXIT_FAILURE;
> }
> -
> - if (f_log) {
> - fclose(f_log);
> - f_log = NULL;
> - }
> closelog();
> return ret;
> }
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel