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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> 
> 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 <dinechin@xxxxxxxxxx>
> ---
>  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(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 &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.

> +  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 &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 ?

>  {
>      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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]