Re: [Spice-devel] [PATCH spice-streaming-agent v2 4/4] Move all stream-related functions within SpiceStream class

[Christophe de Dinechin]

> On 10 Nov 2017, at 14:15, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> 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 
>> ---
>> 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)
>> +  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 
>> +
>> +  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 );
>> +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 )
>> +void SpiceStream::send_cursor(const XFixesCursorImage )
>> {
>> if (image.width >= 1024 || image.height >= 1024)
>> return;
>> @@ -337,7 +344,7 @@ static void send_cursor(const XFixesCursorImage )
>> 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 

Re: [Spice-devel] [PATCH spice-streaming-agent v2 4/4] Move all stream-related functions within SpiceStream class

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> 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 
> ---
>  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.

> -  Stream(const char *name, int ): 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?

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

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

> +  int send_format(unsigned w, unsigned h, unsigned c);
> +  int send_frame(const void *buf, const unsigned size);
> +  void send_cursor(const XFixesCursorImage );
> +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 ?

>  {
>  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 )
> +void SpiceStream::send_cursor(const XFixesCursorImage )
>  {
>  if (image.width >= 1024 || image.height >= 1024)
>  return;
> @@ -337,7 +344,7 @@ static void send_cursor(const XFixesCursorImage )
>  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
>