Re: [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

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

 



On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> 
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later
> patch)
> Fixes a race condition, make sure we only use stream after opening
> 
> Signed-off-by: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> ---
>  src/spice-streaming-agent.cpp | 79 +++++++++++++++++++++++++------
> ------------
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> agent.cpp
> index 646d15b..9b8fd45 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>      StreamMsgData msg;
>  };
>  
> +struct SpiceStreamCursorMessage
> +{
> +    StreamDevHeader hdr;
> +    StreamMsgCursorSet msg;
> +};
> +
> +class Stream
> +{
> +public:
> +    Stream(const char *name)
> +    {
> +        fd = open(name, O_RDWR);
> +        if (fd < 0)
> +            throw std::runtime_error("failed to open streaming
> device");
> +    }
> +    ~Stream()
> +    {
> +        close(fd);
> +    }
> +    operator int() { return fd; }
> +
> +private:
> +    int fd = -1;
> +};


What about making the 'Stream' constructor take an fd so that the same
class can be used to encapsulate e.g. socket fds as well?


> +
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
>  static std::set<SpiceVideoCodecType> client_codecs;
> -static int streamfd = -1;
>  static std::mutex stream_mtx;
>  
> -static int have_something_to_read(int timeout)
> +static int have_something_to_read(int streamfd, int timeout)
>  {
>      struct pollfd pollfd = {streamfd, POLLIN, 0};
>  
> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>      return 0;
>  }
>  
> -static int read_command_from_device(void)
> +static int read_command_from_device(int streamfd)
>  {
>      StreamDevHeader hdr;
>      uint8_t msg[64];
> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>      return 1;
>  }
>  
> -static int read_command(bool blocking)
> +static int read_command(int streamfd, bool blocking)
>  {
>      int timeout = blocking?-1:0;
>      while (!quit_requested) {
> -        if (!have_something_to_read(timeout)) {
> +        if (!have_something_to_read(streamfd, timeout)) {
>              if (!blocking) {
>                  return 0;
>              }
>              sleep(1);
>              continue;
>          }
> -        return read_command_from_device();
> +        return read_command_from_device(streamfd);
>      }
>  
>      return 1;
> @@ -157,7 +181,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)
> +static int spice_stream_send_format(int streamfd, unsigned w,
> unsigned h, unsigned c)
>  {
>  
>      SpiceStreamFormatMessage msg;
> @@ -178,7 +202,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)
> +static int spice_stream_send_frame(int streamfd, const void *buf,
> const unsigned size)
>  {
>      SpiceStreamDataMessage msg;
>      const size_t msgsize = sizeof(msg);
> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(unsigned width, unsigned height, int hotspot_x, int
> hotspot_y,
> +send_cursor(int streamfd, unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>              std::function<void(uint32_t *)> fill_cursor)
>  {
>      if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>      write_all(streamfd, msg.get(), cursor_size);
>  }
>  
> -static void cursor_changes(Display *display, int event_base)
> +static void cursor_changes(int streamfd, Display *display, int
> event_base)
>  {
>      unsigned long last_serial = 0;
>  
> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display,
> int event_base)
>              for (unsigned i = 0; i < cursor->width * cursor->height; 
> ++i)
>                  pixels[i] = cursor->pixels[i];
>          };
> -        send_cursor(cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
> +        send_cursor(streamfd,
> +                    cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
>      }
>  }
>  
>  static void
> -do_capture(const char *streamport, FILE *f_log)
> +do_capture(int streamfd, const char *streamport, FILE *f_log)
>  {
> -    streamfd = open(streamport, O_RDWR);
> -    if (streamfd < 0)
> -        throw std::runtime_error("failed to open the streaming
> device (" +
> -                                 std::string(streamport) + "): "
> -                                 + strerror(errno));
> -
>      unsigned int frame_count = 0;
>      while (!quit_requested) {
>          while (!quit_requested && !streaming_requested) {
> -            if (read_command(true) < 0) {
> +            if (read_command(streamfd, true) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> -                goto done;
> +                return;
>              }
>          }
>  
> @@ -370,7 +389,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 (spice_stream_send_format(streamfd, width,
> height, codec) == EXIT_FAILURE)
>                      throw std::runtime_error("FAILED to send format
> message");
>              }
>              if (f_log) {
> @@ -382,23 +401,18 @@ 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 (spice_stream_send_frame(streamfd,
> +                                        frame.buffer,
> frame.buffer_size) == EXIT_FAILURE) {
>                  syslog(LOG_ERR, "FAILED to send a frame\n");
>                  break;
>              }
>              //usleep(1);
> -            if (read_command(false) < 0) {
> +            if (read_command(streamfd, false) < 0) {
>                  syslog(LOG_ERR, "FAILED to read command\n");
> -                goto done;
> +                return;
>              }
>          }
>      }
> -
> -done:
> -    if (streamfd >= 0) {
> -        close(streamfd);
> -        streamfd = -1;
> -    }
>  }
>  
>  #define arg_error(...) syslog(LOG_ERR, ## __VA_ARGS__);
> @@ -481,12 +495,13 @@ int main(int argc, char* argv[])
>      Window rootwindow = DefaultRootWindow(display);
>      XFixesSelectCursorInput(display, rootwindow,
> XFixesDisplayCursorNotifyMask);
>  
> -    std::thread cursor_th(cursor_changes, display, event_base);
> +    Stream streamfd(streamport);
> +    std::thread cursor_th(cursor_changes, (int) streamfd, display,
> event_base);
>      cursor_th.detach();
>  
>      int ret = EXIT_SUCCESS;
>      try {
> -        do_capture(streamport, f_log);
> +        do_capture(streamfd, streamport, f_log);
>      }
>      catch (std::runtime_error &err) {
>          syslog(LOG_ERR, "%s\n", err.what());
_______________________________________________
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]