> On 10 Nov 2017, at 14:15, Frediano Ziglio <fzig...@redhat.com> wrote:
>
>>
>> From: Christophe de Dinechin <dinec...@redhat.com>
>>
>> This incidentally fixes a race condition processing X events,
>> where we could possibly start sending cursor events to the
>> stream before it was actually open.
>>
>> Signed-off-by: Christophe de Dinechin <dinec...@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 68
>> +++++++++++++++++++++++--------------------
>> 1 file changed, 37 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 8300cf2..33e0345 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -51,31 +51,39 @@ struct SpiceStreamDataMessage
>> StreamMsgData msg;
>> };
>>
>> -struct Stream
>> +struct SpiceStream
>> {
>
> Would promote to a class.
OK
>
>> - Stream(const char *name, int &fd): fd(fd)
>> + SpiceStream(const char *name): streamfd(open(name, O_RDWR))
>> {
>> - fd = open(name, O_RDWR);
>> - if (fd < 0)
>> + if (streamfd < 0)
>> throw std::runtime_error("failed to open streaming device");
>> }
>> - ~Stream()
>> + ~SpiceStream()
>> {
>> - if (fd >= 0)
>> - close(fd);
>> - fd = -1;
>> + if (streamfd >= 0)
>> + close(streamfd);
>> + streamfd = -1;
>
> is the if still needed?
No
>
>> }
>> - int &fd;
>> +
>> + int have_something_to_read(int *pfd, int timeout);
>> + int read_command_from_stdin(void);
>> + int read_command_from_device(void);
>> + int read_command(int blocking);
>
> Some are not strictly related to the stream.
But right now, things are a bit too interconnected because of the way
read_command is structured.
Id’ like to split into another class, but I was planning to do that next.
>
>> + size_t write_all(int fd, const void *buf, const size_t len);
>
> this has nothing to specifically do with the structure,
> mostly with some API (write) behaviour like partial write and
> signals.
Made it private. Also removed fd, it’s always streamfd.
>
>> + int send_format(unsigned w, unsigned h, unsigned c);
>> + int send_frame(const void *buf, const unsigned size);
>> + void send_cursor(const XFixesCursorImage &image);
>> +private:
>> + int streamfd;
>> };
>>
>> static int streaming_requested;
>> static bool quit;
>> -static int streamfd = -1;
>> static bool stdin_ok;
>> static int log_binary = 0;
>> static std::mutex stream_mtx;
>>
>> -static int have_something_to_read(int *pfd, int timeout)
>> +int SpiceStream::have_something_to_read(int *pfd, int timeout)
>> {
>> int nfds;
>> struct pollfd pollfds[2] = {
>> @@ -97,7 +105,7 @@ static int have_something_to_read(int *pfd, int timeout)
>> return *pfd != -1;
>> }
>>
>> -static int read_command_from_stdin(void)
>> +int SpiceStream::read_command_from_stdin(void)
>> {
>> char buffer[64], *p, *save = NULL;
>>
>> @@ -121,7 +129,7 @@ static int read_command_from_stdin(void)
>> return 1;
>> }
>>
>> -static int read_command_from_device(void)
>> +int SpiceStream::read_command_from_device(void)
>> {
>> StreamDevHeader hdr;
>> uint8_t msg[64];
>> @@ -163,7 +171,7 @@ static int read_command_from_device(void)
>> return 1;
>> }
>>
>> -static int read_command(int blocking)
>> +int SpiceStream::read_command(int blocking)
>
> OT: maybe should be bool blocking ?
OK
>
>> {
>> int fd, n=1;
>> int timeout = blocking?-1:0;
>> @@ -185,8 +193,7 @@ static int read_command(int blocking)
>> return n;
>> }
>>
>> -static size_t
>> -write_all(int fd, const void *buf, const size_t len)
>> +size_t SpiceStream::write_all(int fd, const void *buf, const size_t len)
>> {
>> size_t written = 0;
>> while (written < len) {
>> @@ -204,7 +211,7 @@ write_all(int fd, const void *buf, const size_t len)
>> return written;
>> }
>>
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>> +int SpiceStream::send_format(unsigned w, unsigned h, unsigned c)
>> {
>>
>> SpiceStreamFormatMessage msg;
>> @@ -225,7 +232,7 @@ static int spice_stream_send_format(unsigned w, unsigned
>> h, unsigned c)
>> return EXIT_SUCCESS;
>> }
>>
>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>> +int SpiceStream::send_frame(const void *buf, const unsigned size)
>> {
>> SpiceStreamDataMessage msg;
>> const size_t msgsize = sizeof(msg);
>> @@ -304,7 +311,7 @@ static void usage(const char *progname)
>> exit(1);
>> }
>>
>> -static void send_cursor(const XFixesCursorImage &image)
>> +void SpiceStream::send_cursor(const XFixesCursorImage &image)
>> {
>> if (image.width >= 1024 || image.height >= 1024)
>> return;
>> @@ -337,7 +344,7 @@ static void send_cursor(const XFixesCursorImage &image)
>> write_all(streamfd, msg.get(), cursor_size);
>> }
>>
>> -static void cursor_changes(Display *display, int event_base)
>> +static void cursor_changes(Display *display, int event_base, SpiceStream
>> *stream)
>> {
>> unsigned long last_serial = 0;
>>
>> @@ -355,23 +362,25 @@ static void cursor_changes(Display *display, int
>> event_base)
>> continue;
>>
>> last_serial = cursor->cursor_serial;
>> - send_cursor(*cursor);
>> + stream->send_cursor(*cursor);
>> }
>> }
>>
>> static void
>> -do_capture(const char *streamport, FILE *f_log)
>> +do_capture(const char *streamport, FILE *f_log, Display *display, int
>> event_base)
>> {
>> std::unique_ptr<FrameCapture> capture(agent.GetBestFrameCapture());
>> if (!capture)
>> throw std::runtime_error("cannot find a suitable capture system");
>>
>> - Stream stream(streamport, streamfd);
>> + SpiceStream stream(streamport);
>> + std::thread cursor_th(cursor_changes, display, event_base, &stream);
>> + cursor_th.detach();
>>
>> unsigned int frame_count = 0;
>> while (! quit) {
>> while (!quit && !streaming_requested) {
>> - if (read_command(1) < 0) {
>> + if (stream.read_command(1) < 0) {
>> syslog(LOG_ERR, "FAILED to read command\n");
>> return;
>> }
>> @@ -406,7 +415,7 @@ do_capture(const char *streamport, FILE *f_log)
>>
>> syslog(LOG_DEBUG, "wXh %uX%u codec=%u\n", width, height,
>> codec);
>>
>> - if (spice_stream_send_format(width, height, codec) ==
>> EXIT_FAILURE)
>> + if (stream.send_format(width, height, codec) ==
>> EXIT_FAILURE)
>> throw std::runtime_error("FAILED to send format
>> message");
>> }
>> if (f_log) {
>> @@ -417,12 +426,12 @@ do_capture(const char *streamport, FILE *f_log)
>> hexdump(frame.buffer, frame.buffer_size, f_log);
>> }
>> }
>> - if (spice_stream_send_frame(frame.buffer, frame.buffer_size) ==
>> EXIT_FAILURE) {
>> + if (stream.send_frame(frame.buffer, frame.buffer_size) ==
>> EXIT_FAILURE) {
>> syslog(LOG_ERR, "FAILED to send a frame\n");
>> break;
>> }
>> //usleep(1);
>> - if (read_command(0) < 0) {
>> + if (stream.read_command(0) < 0) {
>> syslog(LOG_ERR, "FAILED to read command\n");
>> return;
>> }
>> @@ -516,12 +525,9 @@ int main(int argc, char* argv[])
>> Window rootwindow = DefaultRootWindow(display);
>> XFixesSelectCursorInput(display, rootwindow,
>> XFixesDisplayCursorNotifyMask);
>>
>> - std::thread cursor_th(cursor_changes, display, event_base);
>> - cursor_th.detach();
>> -
>> int ret = EXIT_SUCCESS;
>> try {
>> - do_capture(streamport, f_log);
>> + do_capture(streamport, f_log, display, event_base);
>> }
>> catch (std::runtime_error &err) {
>> syslog(LOG_ERR, "%s\n", err.what());
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
